Re: EDK2 bug report

["Michael D Kinney" <michael.d.kinney@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

Ray,

I agree that removing all UGA content makes the most sense.

Mike

From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, November 18, 2019 11:59 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Jonathan O'Neal <jko@piquantinnovation.com>
Subject: RE: EDK2 bug report

+ Zhichao, Mike and mailing list.

In general, if the code does contain bug handling UGA but isn't found until now, I think we are safe to remove the UGA support now.


From: Jonathan O'Neal <jko@piquantinnovation.com<mailto:jko@piquantinnovation.com>>
Sent: Monday, November 18, 2019 4:52 AM
To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Subject: EDK2 bug report


Hi - sorry to email you directly, but I couldn't find an address for a relevant mailing list, and I wasn't sure how else to report this.

In trying to understand the EDK2 Graphics Console, I think I found an ugly bug (which I might have also encountered in a UGA runtime environment).  In MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c,
GraphicsConsoleControllerDriverStart() does the following circa line 436:

   if (Private->GraphicsOutput != NULL) {
      // console is GOP, handle as GOP
      // ...
   } else {
      // console is UGA
      Status = CheckModeSupported(Private->GraphicsOutput, ...);
      // !! note that Private->GraphicsOutput is known to be NULL here
      // ...
   }

Then, in CheckModeSupported(), circa line 773, the GraphicsOutput pointer is dereferenced without a NULL check:

   MaxMode = GraphicsOutput->Mode->MaxMode;

It appears that CheckModeSupported()only supports GOP, not UGA.  Therefore, GraphicsConsoleControllerDriverStart() should not be calling it when the output device is known to be UGA - and/or, CheckModeSupported() should contain a defensive NULL check prior to dereferencing the GraphicsOutput pointer.

(I realize that UGA is deprecated, but if some level of UGA support remains in the code, it seems like it shouldn't dereference a NULL pointer.)

Sorry if emailing you directly was inappropriate;  if it was, please let me know whom I should contact.

Thanks for your work on EDK.

-Jonathan O'Neal

[-- Attachment #2: Type: text/html, Size: 44985 bytes --]