Out-of-bounds access (OVERRUN) in libeg/load_icns.c >> egDecodeICNS

[dakanji]

Memory Corruption from Out-of-Bounds Access

Checking IconSize == 128ULL implies that IconSize is 128 on the true branch in: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/load_icns.c#L133

Assigning: PixelCount = IconSize * IconSize. The value of PixelCount is now 16384 in: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/load_icns.c#L187

Overrunning buffer pointed to by (UINT8 *)&NewImage->PixelData->a of 1 bytes by passing it to a function which accesses it at byte offset 65532 using argument PixelCount (which evaluates to 16384) in: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/load_icns.c#L219

[joevt]

Not a bug. PixelData is a pointer in EG_IMAGE to one EG_PIXEL but it's usually more than one pixel in memory. It's complaining because egSetPlane is passing a pointer to a single byte - the alpha channel of the first pixel (NewImage->PixelData->a).

Is there documentation somewhere for Coverity? Is there a #pragma or something that can make the warning go away? Can PLPTR be modified to remove the problem? Maybe if it used the offsetof() function? #define PLPTR(imagevar, colorname) ( ((UINT8 *)((imagevar)->PixelData)) + offsetof(EG_PIXEL, colorname) )

[dakanji]

Thanks,

The same function is used for the RGB pixels without issue though. Only difference here seems to be that when WantAlpha is true, there is 255 added. Needs looking at further. Will raise with upstream as well for more insight.

[joevt]

I think for the RGB components, Coverity doesn't know that PixelCount can be 16384 so it doesn't complain.

[dakanji]

Thanks. Makes sense. Will circle back to this later.

[dakanji]

Studied this a bit and pretty sure the current implementation is flawed.

From previous photography dabbling, I know that the Alpha Channel, which is a mask, should be between 0 (black - fully blocks the pixel colours ... that is, transparent) and 255 (white - fully allows the pixel colours ... that is, not transparent) but here, it seems to be doing 255 + 4 which is an invalid value.

'0' and '255' seem to be transposed and also seems the memory pointer thing is wrong (Coverity is correct).

Anyway, re-interpreting the call here to disable the channel and work within the allocated memory.

[joevt]

From previous photography dabbling, I know that the Alpha Channel, which is a mask, should be between 0 (black - fully blocks the pixel colours ... that is, transparent) and 255 (white - fully allows the pixel colours ... that is, not transparent)

If WantAlpha is true, the code wants the Alpha from the icon or image if it exists, or 255 if it doesn't exist. If WantlApha is false then they don't want the Alpha and it should be cleared to 0. Like you say, 0 is not correct if they are using Alpha to draw, but they don't want to use Alpha so it's ok as long as they don't use Alpha.

but here, it seems to be doing 255 + 4 which is an invalid value.

There's no 255 + 4. Where do you see + 4? WantAlpha ? 255 : 0 means 255 if true and 0 if false.

'0' and '255' seem to be transposed

huh?

and also seems the memory pointer thing is wrong (Coverity is correct).

I don't think so.

Anyway, re-interpreting the call here to disable the channel and work within the allocated memory.

You broke the case where they don't want Alpha.

[dakanji]

Thanks for the feedback but quite happy with the state of things

There's no 255 + 4. Where do you see + 4?

May have misinterpreted how egSetPlane is actually using the params passed to it. In any case, not calling it here any longer and now setting each Alpha channel pixel individually with the benefit of not needing to bother about implications of pointing to 1 byte while accessing other stuff (Happier with this whatever that was actually a big issue or not).

WantAlpha ? 255 : 0 means 255 if true and 0 if false.

Correct and this is part of the issue (transposed). Alpha channel pixels set to 255 are basically inactive and those set to 0 are fully active.

You broke the case where they don't want Alpha.

Actually, the Alpha channel was already being set on both parts of the if/else block. 'WantsAlpha' is a bit of a misnomer here in that it doesn't mean the Alpha channel is not wanted, but that transparency (set in the channel) is not wanted.

Key thing is that having the channel does not mean transparency is on and what has been done now is make sure the transparency is off on Alpha channel pixels when 'WantsAlpha' (AKA 'WantsTransparency') is off or when the call does not want it, or when it is wanted but is not available.

This is done by setting each pixel in the channel to 255. The code iterates over each pixel present in the channel and switches the transparency of the pixel off (255 value).

End point is that the RGB Channels are not masked by the Alpha channel and there are definitely no memory management issues regardless of the validity of pointing to single byte as was before.

Going back to the transposing thing, the Alpha Channel is a mask which goes back to analogue photography, where you would take a piece of cardboard in the darkroom and cut holes in it while developing to allow a higher exposure for parts of the image. Pixel Value '0' is equivalent to the solid cardboard parts and Pixel Value '255' is equivalent to a hole.

You could have some semi-transparent paper over the hole which would represent values of 1 to 254 depending on how transparent the paper is.

The channel masks the RGB pixels in the same manner. With 255, you see the full RGB colour (no transparency) and with 0, you see the background (fully transparent).

I need to confirm that the inverted interpretation of 255 and 0 in an Alpha channel has not been negated elsewhere (double negative) and an issue has not been created by fixing it here. Could set the pixels to '0' instead of '255' if needed to match the setup if that turns out to be the case and too difficult to immediately switch things.

[joevt]

I've looked at the format of the t8mk type icon used for the mask of 128x128 icons. It does appear to use 0x00 for transparent and 0xff for opaque.

When WantAlpha is false, it doesn't mean that they want the image to be opaque (alpha = 255) or transparent (alpha = 0). They want the image to not have alpha. Images that use 4 bytes per pixel but don't use alpha usually set the unused byte to 0.

Since there was nothing wrong with the original code, you could add an annotation to make coverity ignore the problem at that line of code: https://community.synopsys.com/s/article/how-to-add-code-annotation-I-do-not-know-which-name-should-exist-with-coverity https://doclazy.wordpress.com/2011/07/14/coverity-suppressing-false-positives-with-cod/

how to add code annotation? I do not know which name should exist with //coverity[]

check main event name from code view of Coverity Connect and add it with //coverity[]

Doclazy's Tech Knowledge

Coverity: Suppressing false positives with code annotations

It’s possible to set false positives to “intentional” by adding an annotation to the code. As an example, suppose the system detects that the x local variable can be NULL when it …

[dakanji]

When WantAlpha is false, it doesn't mean that they want the image to be opaque (alpha = 255) or transparent (alpha = 0). They want the image to not have alpha. Images that use 4 bytes per pixel but don't use alpha usually set the unused byte to 0.

Think it is largely moot here as the end result should be the same when Alpha channel is present and set to Opaque or not present. Also not a user selected option but something to consider I suppose.

One thing though, don't you think the Alpha channel is basically always set here regardless of whether WantsAlpha is true or false and that all the ternary operator on WantsAlpha does is to switch transparency on or off in the else part: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/load_icns.c#L215-L219

Since there was nothing wrong with the original code, you could add an annotation to make coverity ignore the problem at that line of code

True. Also possible to do directly on their web interface but currently prefer to proactively set the values. (Will review later)

Lastly, this is the bit that threw me on the +4. Must be missing something there: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/image.c#L824-L825

[dakanji]

CANCELLED https://github.com/dakanji/RefindPlus/commit/8da356a18c26c7921b2a145e6f60314f3277e045

Leaving as is for now. Will look again later.

[dakanji]

I need to confirm that the inverted interpretation of 255 and 0 in an Alpha channel has not been negated elsewhere (double negative) and an issue has not been created by fixing it here.

Inverted original value gets reversed to correct value here while compositing two copies: https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/image.c#L752-L753

[dakanji]

Reverted changes

[dakanji]

Arrghh. Reverting back to earlier fix for time being.

[joevt]

Think it is largely moot here as the end result should be the same when Alpha channel is present and set to Opaque or not present. Also not a user selected option but something to consider I suppose.

End result is different or at least the meaning differs between the two options. Either the image has alpha with values 0-255 or it doesn't have alpha and the unused byte is 0.

Consider that this is a graphics library. It may include features that are not used by rEFInd or RefindPlus but doesn't mean they should be removed, since the features could be used by a different EFI app. Not having alpha might be weird for rEFInd or RefindPlus but maybe not for other users of the library. There is actually at least one place in rEFInd and RefindPlus where WantAlpha is false. You should check if that code makes sense.

One thing though, don't you think the Alpha channel is basically always set here regardless of whether WantsAlpha is true or false and that all the ternary operator on WantsAlpha does is to switch transparency on or off in the else part:

https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/load_icns.c#L215-L219

First line says "if have valid mask and WantAlpha then copy mask to alpha of image. If the mask is not valid or not WantAlpha then do the else part". The else part says "if wantAlpha then set alpha of image to opaque, otherwise clear the unused byte".

The result is: If WantAlpha then copy mask to alpha if the mask is valid, otherwise set alpha of image to opaque. If not WantAlpha then clear unused byte of each pixel in the image. So it could be written like this:

if (WantAlpha)
    if (MaskPtr != NULL && MaskLen >= PixelCount) 
        egInsertPlane(MaskPtr, PLPTR(NewImage, a), PixelCount);
    else
        egSetPlane(PLPTR(NewImage, a), 255, PixelCount);
else
    egSetPlane(PLPTR(NewImage, a), 0, PixelCount);

but currently prefer to proactively set the values. (Will review later)

It's just weird to remove a call to a function just because of a false positive. You still use the egSetPlane function in other places (for r, g, b). Using the function for a is no different.

Lastly, this is the bit that threw me on the +4. Must be missing something there:

https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/image.c#L824-L825

Maybe the second line would make more sense if it said DestPlanePtr += sizeof(EG_PIXEL); since the destination plane is an array of EG_PIXELs. Just remember that DestPlanePtr doesn't necessarily point to an EG_PIXEL though - it points to one of the channels (r,g,b,a) in an EG_PIXEL.

Inverted original value gets reversed to correct value here while compositing two copies:

https://github.com/dakanji/RefindPlus/blob/c9e64c9428d555ebbd9640711ebc3c6d56c67b96/libeg/image.c#L752-L753

No. What's happening here is that there are two images, Top and Comp. The function wants to draw Top onto Comp using the alpha of Top to decide how much of Top and Comp should be included in a pixel. For example, if Top is 75% opaque then we want 75% of Top and 25% of Comp added together.

Alpha = TopPtr->a;      // eg. a = 192 : want 75% of Top
RevAlpha = 255 - Alpha; // eg. r = 63  : want 25% of Comp

// magical integer arithmetic method (without using divide) of doing "(c*r + t*a) / 255" including rounding because floating point and divide are slow
Temp = (UINTN)CompPtr->b * RevAlpha + (UINTN)TopPtr->b * Alpha + 0x80;
CompPtr->b = (Temp + (Temp >> 8)) >> 8;

[dakanji]

Thanks. I had reviewed and reverted beforehand

[joevt]

One last comment for completeness: EG_IMAGE has a flag called HasAlpha which tells you if the alpha is used or not. The egCreateImage function initializes the alpha to zero in either case. If HasAlpha then all pixels begin as transparent (the image is empty). If not HasAlpha, then the unused alpha byte is zero.

[dakanji]

Yeah. Was on tunnel vision mode thinking of the file format as being RGBA. These have the Alpha values incorporated in the A Channel of the pixel array. ICNS is however RGB plus a separate Alpha Mask which may or may not be present.

The rEFInd code creates all images (EG_IMAGE) as RGBA and sets the A channel to zero when not needed. As you mentioned, it has a flag on whether the Alpha Mask is present or not.

So it could be written like this:

if (WantAlpha)
  if (MaskPtr != NULL && MaskLen >= PixelCount) 
      egInsertPlane(MaskPtr, PLPTR(NewImage, a), PixelCount);
  else
      egSetPlane(PLPTR(NewImage, a), 255, PixelCount);
else
  egSetPlane(PLPTR(NewImage, a), 0, PixelCount);

Correct ... The original does set this out but Clarity Before Brevity as they say.

[dakanji]

Resolved in https://github.com/dakanji/RefindPlus/commit/c01fcd74ed754dbe9ab5a7f43dabbf6002462338 by using suggested offsetof (needed to define this).