Re: [PATCH] SCSI: Fix potential out-of-bounds access in drivers/scsi/sd.c
On 09/06/2013 06:24 PM, Paolo Bonzini wrote: > Il 06/09/2013 17:49, Alan Stern ha scritto: >> This patch fixes an out-of-bounds error in sd_read_cache_type(), found >> by Google's AddressSanitizer tool. When the loop ends, we know that >> "offset" lies beyond the end of the data in the buffer, so no Caching >> mode page was found. In theory it may be present, but the buffer size >> is limited to 512 bytes. >> >> Signed-off-by: Alan Stern >> Reported-by: Dmitry Vyukov >> CC: > > Reviewed-by: Paolo Bonzini > Acked-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: Fix potential out-of-bounds access in drivers/scsi/sd.c
Il 06/09/2013 17:49, Alan Stern ha scritto:
> This patch fixes an out-of-bounds error in sd_read_cache_type(), found
> by Google's AddressSanitizer tool. When the loop ends, we know that
> "offset" lies beyond the end of the data in the buffer, so no Caching
> mode page was found. In theory it may be present, but the buffer size
> is limited to 512 bytes.
>
> Signed-off-by: Alan Stern
> Reported-by: Dmitry Vyukov
> CC:
Reviewed-by: Paolo Bonzini
>
> ---
>
>
> [as1709]
>
>
> drivers/scsi/sd.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> Index: usb-3.11/drivers/scsi/sd.c
> ===
> --- usb-3.11.orig/drivers/scsi/sd.c
> +++ usb-3.11/drivers/scsi/sd.c
> @@ -2419,14 +2419,9 @@ sd_read_cache_type(struct scsi_disk *sdk
> }
> }
>
> - if (modepage == 0x3F) {
> - sd_printk(KERN_ERR, sdkp, "No Caching mode page "
> - "present\n");
> - goto defaults;
> - } else if ((buffer[offset] & 0x3f) != modepage) {
> - sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
> - goto defaults;
> - }
> + sd_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
> + goto defaults;
> +
> Page_found:
> if (modepage == 8) {
> sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] SCSI: Fix potential out-of-bounds access in drivers/scsi/sd.c
This patch fixes an out-of-bounds error in sd_read_cache_type(), found
by Google's AddressSanitizer tool. When the loop ends, we know that
"offset" lies beyond the end of the data in the buffer, so no Caching
mode page was found. In theory it may be present, but the buffer size
is limited to 512 bytes.
Signed-off-by: Alan Stern
Reported-by: Dmitry Vyukov
CC:
---
[as1709]
drivers/scsi/sd.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
Index: usb-3.11/drivers/scsi/sd.c
===
--- usb-3.11.orig/drivers/scsi/sd.c
+++ usb-3.11/drivers/scsi/sd.c
@@ -2419,14 +2419,9 @@ sd_read_cache_type(struct scsi_disk *sdk
}
}
- if (modepage == 0x3F) {
- sd_printk(KERN_ERR, sdkp, "No Caching mode page "
- "present\n");
- goto defaults;
- } else if ((buffer[offset] & 0x3f) != modepage) {
- sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
- goto defaults;
- }
+ sd_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
+ goto defaults;
+
Page_found:
if (modepage == 8) {
sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
On 09/04/2013 05:42 PM, Alan Stern wrote:
> On Wed, 4 Sep 2013, Paolo Bonzini wrote:
>
>>> --- 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.
>
> That's right. I didn't realize it at first, but the only way to get
> here is if the next page offset lies beyond the end of the data in the
> buffer. Therefore the patch can be simplified as follows.
>
Oh, pretty please. I already had customers complaining about the
'got wrong page' message. Which again demonstrated nicely the
uncertainty relation between correctness and usability :-)
> 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,14 +2419,9 @@ sd_read_cache_type(struct scsi_disk *sdk
> }
> }
>
> - if (modepage == 0x3F) {
> - sd_printk(KERN_ERR, sdkp, "No Caching mode page "
> - "present\n");
> - goto defaults;
> - } else if ((buffer[offset] & 0x3f) != modepage) {
> - sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
> - goto defaults;
> - }
> + sd_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
> + goto defaults;
> +
> Page_found:
> if (modepage == 8) {
> sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Acked-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
On Wed, 4 Sep 2013, Dmitry Vyukov wrote: > Thanks, Alan! > > I agree with Paolo that the branch can be removed. > > Will you take care of landing the patch? I will when everyone agrees it is ready. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
On Wed, Sep 4, 2013 at 6:32 PM, Alan Stern wrote:
> 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.
Thanks, Alan!
I agree with Paolo that the branch can be removed.
Will you take care of landing the patch?
> 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;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
On Wed, 4 Sep 2013, Paolo Bonzini wrote:
> > --- 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.
That's right. I didn't realize it at first, but the only way to get
here is if the next page offset lies beyond the end of the data in the
buffer. Therefore the patch can be simplified as follows.
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,14 +2419,9 @@ sd_read_cache_type(struct scsi_disk *sdk
}
}
- if (modepage == 0x3F) {
- sd_printk(KERN_ERR, sdkp, "No Caching mode page "
- "present\n");
- goto defaults;
- } else if ((buffer[offset] & 0x3f) != modepage) {
- sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
- goto defaults;
- }
+ sd_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
+ goto defaults;
+
Page_found:
if (modepage == 8) {
sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Potential out-of-bounds access in drivers/scsi/sd.c
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Potential out-of-bounds access in drivers/scsi/sd.c
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.
Here one of the reports from the tool:
[ 166.124485] ERROR: AddressSanitizer: heap-buffer-overflow on
address 880024ab8600
[ 166.126613] 880024ab8600 is located 0 bytes to the right of
512-byte region [880024ab8400, 880024ab8600)
[ 166.129583] Accessed by thread T2505:
[ 166.130732] #0 inlined describe_heap_address
./arch/x86/mm/asan/report.c:164
[ 166.130732] #0 810dd277 (asan_report_error+0x2f7/0x400)
./arch/x86/mm/asan/report.c:278
[ 166.132475] #1 810dc6a0 (asan_check_region+0x30/0x40)
./arch/x86/mm/asan/asan.c:37
[ 166.134485] #2 810dd3c3 (__tsan_read1+0x13/0x20) ??:0
[ 166.136397] #3 inlined sd_read_cache_type ./drivers/scsi/sd.c:2426
[ 166.136397] #3 81675546
(sd_revalidate_disk+0x23d6/0x28d0) ./drivers/scsi/sd.c:2720
[ 166.138531] #4 812f437b (revalidate_disk+0x4b/0xc0)
./fs/block_dev.c:975
[ 166.140449] #5 8167143e (sd_rescan+0x3e/0x50)
./drivers/scsi/sd.c:1473
[ 166.142539] #6 8162c164 (scsi_rescan_device+0x64/0x90)
./drivers/scsi/scsi_scan.c:1566
[ 166.144557] #7 81694e65 (ata_scsi_dev_rescan+0xf5/0x170)
./drivers/ata/libata-scsi.c:3986
[ 166.146680] #8 inlined trace_workqueue_execute_end
./kernel/workqueue.c:2186
[ 166.146680] #8 8640 (process_one_work+0x2d0/0x750)
./kernel/workqueue.c:2191
[ 166.149012] #9 8d23 (worker_thread+0x263/0x640)
./include/linux/list.h:188
[ 166.151061] #10 8111c092 (kthread+0x132/0x140) kthread.c:0
[ 166.152853] #11 8192841c (ret_from_fork+0x7c/0xb0)
./arch/x86/kernel/entry_64.S:570
[ 166.154790]
[ 166.155389] Allocated by thread T2505:
[ 166.156801] #0 810dc768 (asan_slab_alloc+0x48/0xb0)
./arch/x86/mm/asan/asan.c:91
[ 166.158850] #1 inlined slab_alloc ./mm/slab.c:3475
[ 166.158850] #1 inlined __do_kmalloc ./mm/slab.c:3749
[ 166.158850] #1 812832ec (__kmalloc+0xbc/0x500) ./mm/slab.c:3763
[ 166.161364] #2 81673234 (sd_revalidate_disk+0xc4/0x28d0)
./drivers/scsi/sd.c:2698
[ 166.164302] #3 812f437b (revalidate_disk+0x4b/0xc0)
./fs/block_dev.c:975
[ 166.167012] #4 8167143e (sd_rescan+0x3e/0x50)
./drivers/scsi/sd.c:1473
[ 166.169478] #5 8162c164 (scsi_rescan_device+0x64/0x90)
./drivers/scsi/scsi_scan.c:1566
[ 166.172321] #6 81694e65 (ata_scsi_dev_rescan+0xf5/0x170)
./drivers/ata/libata-scsi.c:3986
[ 166.175156] #7 inlined trace_workqueue_execute_end
./kernel/workqueue.c:2186
[ 166.175156] #7 8640 (process_one_work+0x2d0/0x750)
./kernel/workqueue.c:2191
[ 166.177222] #8 8d23 (worker_thread+0x263/0x640)
./include/linux/list.h:188
[ 166.179193] #9 8111c092 (kthread+0x132/0x140) kthread.c:0
[ 166.180988] #10 8192841c (ret_from_fork+0x7c/0xb0)
./arch/x86/kernel/entry_64.S:570
[ 166.182931]
[ 166.183537] Shadow bytes around the buggy address:
[ 166.185391] 880024ab8380: fa fa fa fa fa fa fa fa fa fa fa fa
fa fa fa fa
[ 166.187758] 880024ab8400: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 166.190227] 880024ab8480: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 166.192684] 880024ab8500: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 166.195152] 880024ab8580: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 166.197649] =>880024ab8600:[fa]fa fa fa fa fa fa fa fa fa fa fa
fa fa fa fa
[ 166.199128] 880024ab8680: fa fa fa fa fa fa fa fa fa fa fa fa
fa fa fa fa
[ 166.200962] 880024ab8700: fa fa fa fa fa fa fa fa fd fd fd fd
fd fd fd fd
[ 166.203069] 880024ab8780: fd fd fd fd fd fd fd fd fd fd fd fd
fd fd fd fd
[ 166.205202] 880024ab8800: fd fd fd fd fd fd fd fd fd fd fd fd
fd fd fd fd
[ 166.207302] 880024ab8880: fd fd fd fd fd fd fd fd fd fd fd fd
fd fd fd fd
[ 166.209237] Shadow byte legend (one shadow byte represents 8
application bytes):
[ 166.211273] Addressable: 00
[ 166.212358] Partially addressable: 01 02 03 04 05 06 07
[ 166.213871] Heap redzone: fa
[ 166.214979] Heap kmalloc redzone: fb
[ 166.216062] Freed heap region: fd
[ 166.217149] Shadow gap:fe
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.
--
To unsubscribe from this list: send the line "u
