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

[Andrew Fish <afish@apple.com>]

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? 

Thanks,

Andrew Fish

https://tianocore.acgmultimedia.com/show_bug.cgi?id=105