[PATCH 1/4] staging: comedi: vmk80xx: fix out-of-bounds write

[Hartley Sweeten]

On Monday, February 23, 2015 3:56 AM, Ian Abbott wrote:
> On 20/02/15 19:52, H Hartley Sweeten wrote:
>> usb_blk_msg() will return the passed 'len' (64) as the 'actual_len' (cnt) of
>
> That's usb_bulk_msg().
>
>> the transfer. The addition of the '\0' to the end of the returned string will
>> overrun the 'rx' array. Increase the array size by 1 to fix the out-of-bounds
>> write.
>>
>> Reported-by: coverity (CID 711413)
>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>> Cc: Ian Abbott <abbotti at mev.co.uk>
>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> ---
>>   drivers/staging/comedi/drivers/vmk80xx.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/vmk80xx.c b/drivers/staging/comedi/drivers/vmk80xx.c
>> index e371183..e0656d1 100644
>> --- a/drivers/staging/comedi/drivers/vmk80xx.c
>> +++ b/drivers/staging/comedi/drivers/vmk80xx.c
>> @@ -195,7 +195,7 @@ static void vmk80xx_read_eeprom(struct comedi_device *dev, int flag)
>>   	unsigned int tx_pipe;
>>   	unsigned int rx_pipe;
>>   	unsigned char tx[1];
>> -	unsigned char rx[64];
>> +	unsigned char rx[65];
>>   	int cnt;
>>
>>   	tx_pipe = usb_sndbulkpipe(usb, 0x01);
>>
>
> cnt could be a junk value if the call to usb_bulk_msg() fails. 
> Initializing cnt to 0 should fix that.
>
> You could just check cnt is in range before setting rx[cnt] to '\0'. 
> Better yet, since the subsequent strncpy() copies up to 24 chars 
> starting at either rx + 1 or rx + 25, the tail end of rx[] could be 
> zero-filled after returning from usb_bulk_msg().  Something like:
>
>	if (cnt < 64)
>		memset(rx + cnt, 0, 64 - cnt);
>
> (Ideally, this driver should check the returns from usb_bulk_msg() for 
> errors, but that would be a separate set of patches.  In this case, the 
> information extracted is only used in a kernel log message.)

Ian,

Actually, since the information is just used in kernel log messages can we
just remove it?

It looks like the vmk80xx_read_eeprom() and vmk80xx_check_data_link()
functions could go away along with the struct firmware_version in the
private data.

Regards,
Hartley