Re: [PATCH RESEND] drivers: video: metronomefb: avoid out-of-bounds array access

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On 2013-11-29 18:51, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
> 
> load_waveform function checks whether padding bytes in stuff2a
> and stuff2b are all zero, but does so by treating those arrays
> as a single longer array.  Since the structure is packed, and
> the size sum matches, it all works, but creates some confusion
> in the code.
> 
> This commit changes the stuff2a and stuff2b arrays into pad1 and
> pad2 fields such that they cover the same bytes as the arrays
> covered, and changes the check in the load_waveform function so
> that the fields are read instead of iterating over an arary.
> 
> It also renames the other “stuff” fields to “ignore*” fields to
> give them more semantic meaning.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>  drivers/video/metronomefb.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
> index 195cc2d..4f36a2b 100644
> --- a/drivers/video/metronomefb.c
> +++ b/drivers/video/metronomefb.c
> @@ -126,7 +126,7 @@ static struct fb_var_screeninfo metronomefb_var = {
>  
>  /* the waveform structure that is coming from userspace firmware */
>  struct waveform_hdr {
> -	u8 stuff[32];
> +	u8 ignore1[32];
>  
>  	u8 wmta[3];
>  	u8 fvsn;
> @@ -134,13 +134,14 @@ struct waveform_hdr {
>  	u8 luts;
>  	u8 mc;
>  	u8 trc;
> -	u8 stuff3;
> +	u8 ignore2;
>  
>  	u8 endb;
>  	u8 swtb;
> -	u8 stuff2a[2];
> +	u32 pad1; /* u16 halfof(pad1) */
>  
> -	u8 stuff2b[3];
> +	/* u16 halfof(pad1) */
> +	u8 pad2;

I don't quite follow what those comments mean...

I know nothing of the hw in question, but I guess there's some reason
the stuff2a and stuff2b has been separate, and there's even a blank line
between. If they pad a particular block in the data structure, doesn't
your change make it confusing?

It wouldn't be too pad to have those 5 fields as separate ones. One u16
for the stuff2a, and u16 + u8 for the stuff2b. It'd be simple to check
that those are zero.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]