Re: Potential out-of-bounds access in drivers/scsi/sd.c

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/09/2013 16:32, Alan Stern ha scritto:
> On Wed, 4 Sep 2013, Dmitry Vyukov wrote:
> 
>> Hi,
>>
>> We are working on a memory error detector AddressSanitizer for Linux
>> kernel (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel),
>> it can detect use-after-free and buffer-overflow errors.
> 
> ...
> 
>> The code in sd_read_cache_type does the following:
>>
>> while (offset < len) {
>> ...
>> }
>> ...
>> if ((buffer[offset] & 0x3f) != modepage) {
>>     sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
>>     goto defaults;
>> }
>>
>> When control leaves the while loop, offset >= len, so buffer[offset]
>> reads random garbage out-of-bounds.
>> It the worst case it can lead to crash, or if (buffer[offset] & 0x3f)
>> happen to be == modepage, then it will read more garbage.
>>
>> Please help validate and triage this.
> 
> The tool's output is correct.  The patch below should fix it.
> 
> Alan Stern
> 
> 
> 
> Index: usb-3.11/drivers/scsi/sd.c
> ===================================================================
> --- usb-3.11.orig/drivers/scsi/sd.c
> +++ usb-3.11/drivers/scsi/sd.c
> @@ -2419,7 +2419,7 @@ sd_read_cache_type(struct scsi_disk *sdk
>  			}
>  		}
>  
> -		if (modepage == 0x3F) {
> +		if (modepage == 0x3F || offset + 2 >= len) {
>  			sd_printk(KERN_ERR, sdkp, "No Caching mode page "
>  				  "present\n");
>  			goto defaults;

If you do this, the buggy "if" becomes dead code (the loop above doesn't
have any "break", so you know that offset >= len and the new condition
is always true).

So the patch does indeed prevent the bug, but the code can be simplified.

Paolo