video: fbdev: metronomefb: two harmless off by one bugs

PATCH In review Dan Carpenter 2 participants 2 messages Jan 30, 2016 – Feb 16, 2016

Subsystems (2)

Kernel janitors Framebuffer layer

lore

Share & tools

lore

Dan Carpenter

author

2016-01-30 14:44 UTC #0 lore raw

PATCH 1/1 video: fbdev: metronomefb: two harmless off by one bugs

par->metromem_cmd->args[] is an array of 31 elements of size u16. Here we have initialized the first "i" elements and want to set the rest to zero.

The issue here is that ARRAY_SIZE(par->metromem_cmd->args) is 31 and not 32 as in the original code. It means that we set ->csum to zero, but that is harmless because we immediately set it to the correct value on the next line.

Still, the buffer overflow upsets static checkers so let's correct the math.

Signed-off-by Dan Carpenter <[email protected]>

drivers/video/fbdev/metronomefb.c +4 -2

index ad04a01..abb6bbf 100644
@@ -354,7 +354,8 @@ static int metronome_powerup_cmd(struct metronomefb_par *par)
 	}
 
 	/* the rest are 0 */
-	memset((u8 *) (par->metromem_cmd->args + i), 0, (32-i)*2);
+	memset(&par->metromem_cmd->args[i], 0,
+	       (ARRAY_SIZE(par->metromem_cmd->args) - i) * 2);
 
 	par->metromem_cmd->csum = cs;

@@ -376,7 +377,8 @@ static int metronome_config_cmd(struct metronomefb_par *par)
 	memcpy(par->metromem_cmd->args, epd_frame_table[par->dt].config,
 		sizeof(epd_frame_table[par->dt].config));
 	/* the rest are 0 */
-	memset((u8 *) (par->metromem_cmd->args + 4), 0, (32-4)*2);
+	memset(&par->metromem_cmd->args[4], 0,
+	       (ARRAY_SIZE(par->metromem_cmd->args) - 4) * 2);
 
 	par->metromem_cmd->csum = 0xCC10;
 	par->metromem_cmd->csum += calc_img_cksum(par->metromem_cmd->args, 4);

Tomi Valkeinen

2016-02-16 12:53 UTC #1 lore raw

Quoted 11 lines · Dan Carpenter

par->metromem_cmd->args[] is an array of 31 elements of size u16. Here we have initialized the first "i" elements and want to set the rest to zero.
The issue here is that ARRAY_SIZE(par->metromem_cmd->args) is 31 and not 32 as in the original code. It means that we set ->csum to zero, but that is harmless because we immediately set it to the correct value on the next line.
Still, the buffer overflow upsets static checkers so let's correct the math.

Thanks, queued for 4.6.

Tomi