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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 9 Jan 2014 14:21:58 +0200
From:	Tomi Valkeinen <tomi.valkeinen@...com>
To:	Michal Nazarewicz <mpn@...gle.com>
CC:	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	<linux-fbdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND] drivers: video: metronomefb: avoid out-of-bounds
 array access

On 2013-11-29 18:51, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@...a86.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@...a86.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



Download attachment "signature.asc" of type "application/pgp-signature" (902 bytes)