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

From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: Andrew Fish <afish@apple.com>, edk2-devel <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
	"Carsey, Jaben" <jaben.carsey@intel.com>
Subject: Re: I found a fun bug in the Shell today. Looks like we have been getting lucky?
Date: Thu, 25 Aug 2016 16:05:59 +0000	[thread overview]
Message-ID: <CB6E33457884FA40993F35157061515C54A2D473@FMSMSX103.amr.corp.intel.com> (raw)
In-Reply-To: <B883F72C-4CCC-4867-A2AA-5D1D13E958AE@apple.com>

Andrew,

Can you file a Bugzilla issue so we can track this issue properly?

-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