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:17 AM, Carsey, Jaben <jaben.carsey@intel.com> wrote:
> 
> Doh!  Thanks!  I didn't realize that was the bug.  I will have to finally learn that servers name one of these days...
> 

No worries I thought the URL would be more obvious, I should have labeled it.

Thanks,

Andrew Fish

>> -----Original Message-----
>> From: afish@apple.com [mailto:afish@apple.com]
>> Sent: Thursday, August 25, 2016 9:08 AM
>> To: Carsey, Jaben <jaben.carsey@intel.com>
>> Cc: edk2-devel <edk2-devel@lists.01.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] I found a fun bug in the Shell today. Looks like we have
>> been getting lucky?
>> Importance: High
>> 
>> 
>>> 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
>