Clarification on color parameter in drm_draw_fill24 function

Clarification on color parameter in drm_draw_fill24 function

[Dheeraj Reddy Jonnalagadda]

Dear Maintainers,

I am writing to seek clarification regarding the implementation of the
drm_draw_fill24 function in the DRM subsystem. Specifically, Coverity has
flagged (CID 1602416) the issue in the following line in drm/drm_draw.c

--> iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);

I have some questions about handling of the color parameter in the function.

The function currently accepts a u16 value as the color parameter and uses
bitwise operations to extract the RGB components. However, the mask 0x00FF0000
refers to bits 16–23, which are always zero for a u16 value. Therefore, the
expression (color & 0x00FF0000) will always result in 0.

Could you please confirm:

  1. Is the truncation of 32-bit color value to 16 bits the intended behavior?
  2. Alternatively, should the function be updated to accept 32-bit values
     as input as the function is called with 32 bit values elsewhere?

Thank you for your time. Please let me know if further information or context
is required.

-Dheeraj

follow-up: Clarification on color parameter in drm_draw_fill24 function

[Dheeraj Reddy Jonnalagadda]

On Thu, Dec 12, 2024 at 05:10:21PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> Dear Maintainers,
> 
> I am writing to seek clarification regarding the implementation of the
> drm_draw_fill24 function in the DRM subsystem. Specifically, Coverity has
> flagged (CID 1602416) the issue in the following line in drm/drm_draw.c
> 
> --> iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);
> 
> I have some questions about handling of the color parameter in the function.
> 
> The function currently accepts a u16 value as the color parameter and uses
> bitwise operations to extract the RGB components. However, the mask 0x00FF0000
> refers to bits 16–23, which are always zero for a u16 value. Therefore, the
> expression (color & 0x00FF0000) will always result in 0.
> 
> Could you please confirm:
> 
>   1. Is the truncation of 32-bit color value to 16 bits the intended behavior?
>   2. Alternatively, should the function be updated to accept 32-bit values
>      as input as the function is called with 32 bit values elsewhere?
> 
> Thank you for your time. Please let me know if further information or context
> is required.
> 
> -Dheeraj

Dear Maintainers,

I am following up on my earlier query regarding the potential issue flagged by
Coverity (CID 1602416) in the drm_draw_fill24 function in drm/drm_draw.c.

Link to the Coverity issue:
https://scan7.scan.coverity.com/#/project-view/52348/11354?selectedIssue=1602416

The action field on Coverity has also been changed to "Fix required" for
this issue.

To summarize:

	1. The color parameter is declared as a u16, but the bitwise operation
	   (color & 0x00FF0000) targets bits that do not exist within a u16
	   range, effectively always resulting in 0.

	2. My query is whether this truncation is intentional or if the
	   function should be updated to accept a 32-bit color value, aligning
	   with its usage elsewhere.

I wanted to kindly ask if there has been any decision or additional insight
regarding this issue. If needed, I can provide a proposed patch to address the
problem once the intended behavior is clarified.

Thank you again for your time.

-Dheeraj