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

[Andrew Fish <afish@apple.com>]

> On Aug 25, 2016, at 9:05 AM, Carsey, Jaben <jaben.carsey@intel.com> wrote:
> 
> Andrew,
> 
> Can you file a Bugzilla issue so we can track this issue properly?
> 

Jaben,

I attached the URL at the end of the original mail: https://tianocore.acgmultimedia.com/show_bug.cgi?id=105

Thanks,

Andrew Fish

> -Jaben
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Andrew Fish
>> Sent: Wednesday, August 24, 2016 6:26 PM
>> To: edk2-devel <edk2-devel@lists.01.org>
>> Subject: Re: [edk2] I found a fun bug in the Shell today. Looks like we have
>> been getting lucky?
>> Importance: High
>> 
>> 
>>> 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
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel