Re: I found a fun bug in the Shell today. Looks like we have been getting lucky?

[Andrew Fish <afish@apple.com>]

> On Aug 24, 2016, at 5:59 PM, Andrew Fish <afish@apple.com> wrote:
> 
> I was tracking down a data corruption issue when paging was enabled on an edk2 shell command. The crash was in a custom ConSpliter over writing a DXE Core data structure. The buffer overflow seemed to be caused by the Console getting confused on the location of the end of the screen. I set a watchpoint on gST->ConOut->Mode->CursorRow and found the shell was the one corrupting the Mode data. 
> 
> UEFI Spec: The following data values in the SIMPLE_TEXT_OUTPUT_MODE interface are read-only and are changed by using the appropriate interface functions:
> 
> (master)>git grep "OurConOut.Mode"
> Application/Shell/ConsoleLogger.c:72:  (*ConsoleInfo)->OurConOut.Mode              = gST->ConOut->Mode;
> Application/Shell/ConsoleLogger.c:647://    ShellInfoObject.ConsoleInfo->OurConOut.Mode->CursorRow    = 0;
> Application/Shell/ConsoleLogger.c:648://    ShellInfoObject.ConsoleInfo->OurConOut.Mode->CursorColumn = 0;
> Application/Shell/ConsoleLogger.c:704:      if (ConsoleInfo->OurConOut.Mode->CursorColumn > 0) {
> Application/Shell/ConsoleLogger.c:705:        ConsoleInfo->OurConOut.Mode->CursorColumn--;
> Application/Shell/ConsoleLogger.c:734:      ConsoleInfo->OurConOut.Mode->CursorRow++;
> Application/Shell/ConsoleLogger.c:741:      ConsoleInfo->OurConOut.Mode->CursorColumn = 0;
> Application/Shell/ConsoleLogger.c:747:      ConsoleInfo->OurConOut.Mode->CursorColumn++;
> Application/Shell/ConsoleLogger.c:751:      if ((INTN)ConsoleInfo->ColsPerScreen == ConsoleInfo->OurConOut.Mode->CursorColumn + 1) {
> Application/Shell/ConsoleLogger.c:781:        ConsoleInfo->OurConOut.Mode->CursorRow++;
> Application/Shell/ConsoleLogger.c:782:        ConsoleInfo->OurConOut.Mode->CursorColumn = 0;
> Application/Shell/ConsoleLogger.c:976:    ConsoleInfo->OurConOut.Mode = ConsoleInfo->OldConOut->Mode;
> 
> 
> I'm not exactly sure what this code is trying to do as the console should update Mode structure directly? Maybe the intent was to have a copy of gST->ConOut->Mode and keep it in sync? It seems like this should cause more issues, but maybe the edk2 ConSplitter is not broken by this behavior and we are getting lucky? 
> 

I forgot to mention that setting the Mode->CursorRow in the console code back to the last row if was larger looks like it hides this bug in the shell. 

Thanks,

Andrew Fish


> Thanks,
> 
> Andrew Fish
> 
> https://tianocore.acgmultimedia.com/show_bug.cgi?id=105
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel