bug report: sd: off by one in sd_read_block_limits()
Subsystems (2)
drivers/scsi/sd.c +1986 sd_read_block_limits(39) warn: buffer overflow 'buffer' 32 <= 32
1951 const int vpd_len = 32;
1952 unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
[snip]
1984 if (buffer[32] & 0x80)
This is past the end of the array.
1985 q->limits.discard_alignment 1986 get_unaligned_be32(&buffer[32]) & ~(1 << 31); 1987 }
regards,
dan carpenter
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" = Dan Carpenter <[email protected]> writes:
Dan> drivers/scsi/sd.c +1986 sd_read_block_limits(39) warn: buffer Dan> overflow 'buffer' 32 <= 32
Dan> 1951 const int vpd_len = 32;
sd: Fix block limits VPD page length
Commit e3deec09 incorrectly assumed that the page length was limited to 32 bytes. The B0 VPD page length is defined to be 60 bytes when the device supports thin provisioning.
---
drivers/scsi/sd.c +1 -1
@@ -1948,7 +1948,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) {struct request_queue *q = sdkp->disk->queue;unsigned int sector_sz = sdkp->device->sector_size; -const int vpd_len =32 ; +const int vpd_len =60 ;unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL ); if (!buffer ||
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Martin" = Martin K Petersen <[email protected]> writes:
Actually, looking closer at the VPD mechanism in e3deec09 we need to fit the page header as well...
sd: Fix block limits VPD page length
Commit e3deec09 incorrectly assumed that the page length was limited to 32 bytes. The B0 VPD page length is defined to be 64 bytes when the device supports thin provisioning.
---
drivers/scsi/sd.c +1 -1
@@ -1948,7 +1948,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) {struct request_queue *q = sdkp->disk->queue;unsigned int sector_sz = sdkp->device->sector_size; -const int vpd_len =32 ; +const int vpd_len =64 ;unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL ); if (!buffer ||
Quoted 4 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Martin" = Martin K Petersen <[email protected]> writes:
Actually, looking closer at the VPD mechanism in
e3deec09we need to fit the page header as well...
These aren't the only two bugs introduced either. If a device has more VPD pages than the length of the buffer that the caller has allocated, we will now incorrectly report that the device doesn't support that page.
James, was any problem actually observed that led to e3deec09? I really think you should revert it.
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
James,
While I agree the original VPD code's double kmalloc() was a bit of a wart at least it did the right thing because it allocated a suitably sized buffer for page 0.
My concern with the interface you introduced in e3deec09 is that for devices that support a large number of VPD pages we won't be able to fit the page list in the allocated buffer. And callers are likely to pick a buffer size that makes sense for the VPD page they are interested in. It's not a big deal in the block limits/block device characteristics case because they are big enough.
But at the very minimum that interface should come with a big fat warning in the comment section that describes that the page list must also be able to fit.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Martin" = Martin K Petersen <[email protected]> writes:
Page B1 is also defined to be 64 bytes long...
sd: Fix VPD buffer allocations
Commit e3deec09 incorrectly assumed that the B0 and B1 page lengths were limited to 32 bytes. The B0 VPD page length is defined to be 64 bytes when the device supports thin provisioning. B1 is always defined to be 64 bytes.
drivers/scsi/sd.c +2 -2
@@ -1948,7 +1948,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) {struct request_queue *q = sdkp->disk->queue;unsigned int sector_sz = sdkp->device->sector_size; -const int vpd_len =32 ; +const int vpd_len =64 ;unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL ); if (!buffer ||
@@ -1998,7 +1998,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp) {unsigned char *buffer;u16 rot; -const int vpd_len =32 ; +const int vpd_len =64 ; buffer =kmalloc (vpd_len,GFP_KERNEL );
Quoted 11 lines · Martin K. Petersen
While I agree the original VPD code's double
kmalloc()was a bit of a wart at least it did the right thing because it allocated a suitably sized buffer for page 0.My concern with the interface you introduced in
e3deec09is that for devices that support a large number of VPD pages we won't be able to fit the page list in the allocated buffer. And callers are likely to pick a buffer size that makes sense for the VPD page they are interested in. It's not a big deal in the block limits/block device characteristics case because they are big enough.
Quoted 3 lines
But at the very minimum that interface should come with a big fat warning in the comment section that describes that the page list must also be able to fit.
Here's a patch that should address everyone's concerns.
----
From 88e1cb1368f3b204be5ba800c0b8b91482233c70 Mon Sep 17 00:00:00 2001
From: Matthew Wilcox <[email protected]>
Date: Tue, 2 Mar 2010 09:40:18 -0500
Subject: [PATCH] [SCSI] Fix multiple bugs in scsi_get_vpd_page()
The rewrite in commit e3deec09 introduced a number of new bugs. Revert to the old algorithm, and add the ability for callers to specify that they don't need the entire page, just the first N bytes. This eliminates the kmalloc failure potential that James saw.
--- drivers/scsi/scsi.c | 47 ++++++++++++++++++++++++++----------- drivers/scsi/scsi_transport_sas.c | 7 ++--- drivers/scsi/sd.c | 26 ++++++++------------ drivers/scsi/ses.c | 10 ++----- include/scsi/scsi_device.h | 4 +- 5 files changed, 51 insertions(+), 43 deletions(-)
drivers/scsi/scsi.c +33 -14
@@ -1018,8 +1018,7 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,* scsi_get_vpd_page - Get Vital Product Data from a SCSI device * @sdev: The device to ask * @page: Which Vital Product Data to return -* @buf: where to store the VPD -* @buf_len: number of bytes in the VPD buffer area +* @len: number of bytes requested from the VPD page * * SCSI devices may optionally supply Vital Product Data. Each 'page' * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
@@ -1028,39 +1027,59 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,* responsible for calling kfree() on this pointer when it is no longer * needed. If we cannot retrieve the VPD page this routine returns %NULL. */ -int scsi_get_vpd_page (struct scsi_device *sdev,u8 page,unsigned char *buf, -int buf_len) +unsigned char *scsi_get_vpd_page (struct scsi_device *sdev,u8 page, +unsigned len) {int i, result; +unsigned char *buf; +unsigned init_vpd_len =max (len,255U ); + + buf =kmalloc (init_vpd_len,GFP_KERNEL ); +if (!buf) +return NULL ;/* Ask for all the pages supported by this device */ - result =scsi_vpd_inquiry (sdev, buf,0 , buf_len); + result =scsi_vpd_inquiry (sdev, buf,0 ,255 );if (result) goto fail;/* If the user actually wanted this page, we can skip the rest */ if (page =0 ) -return -EINVAL ; +return buf; -for (i =0 ; i <min ((int )buf[3 ], buf_len -4 ); i++) +for (i =0 ; i < buf[3 ]; i++)if (buf[i +4 ] = page) goto found; - -if (i < buf[3 ] && i > buf_len) -/* ran off the end of the buffer, give us benefit of doubt */ - goto found;/* The device claims it doesn't support the requested page */ goto fail; found: - result =scsi_vpd_inquiry (sdev, buf, page, buf_len); + result =scsi_vpd_inquiry (sdev, buf, page, len);if (result) goto fail; -return 0 ; +if (len >0 ) +return buf; + + len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; +if (len <= init_vpd_len) +return buf; + +/* Caller asked us to allocate the correct amount */ +kfree (buf); + buf =kmalloc (len,GFP_KERNEL ); +if (!buf) +return NULL ; + + result =scsi_vpd_inquiry (sdev, buf, page, len); +if (result) + goto fail; + +return buf; fail: -return -EINVAL ; +kfree (buf); +return NULL ; }EXPORT_SYMBOL_GPL (scsi_get_vpd_page);
drivers/scsi/scsi_transport_sas.c +3 -4
@@ -380,12 +380,12 @@ EXPORT_SYMBOL(sas_remove_host);unsigned int sas_tlr_supported (struct scsi_device *sdev) { -const int vpd_len =32 ;struct sas_end_device *rdev =sas_sdev_to_rdev (sdev); -char *buffer =kzalloc (vpd_len,GFP_KERNEL ); +char *buffer;int ret =0 ; -if (scsi_get_vpd_page (sdev,0x90 , buffer, vpd_len)) + buffer =scsi_get_vpd_page (sdev,0x90 ,32 ); +if (!buffer) goto out;/*
@@ -400,7 +400,6 @@ sas_tlr_supported(struct scsi_device *sdev)kfree (buffer); rdev->tlr_supported = ret;return ret; - }EXPORT_SYMBOL_GPL (sas_tlr_supported);
drivers/scsi/sd.c +10 -16
@@ -1948,13 +1948,12 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) {struct request_queue *q = sdkp->disk->queue;unsigned int sector_sz = sdkp->device->sector_size; -const int vpd_len =32 ; -unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL ); +char *buffer; -if (!buffer || -/* Block Limits VPD */ -scsi_get_vpd_page (sdkp->device,0xb0 , buffer, vpd_len)) - goto out; +/* Block Limits VPD */ + buffer =scsi_get_vpd_page (sdkp->device,0xb0 ,64 ); +if (!buffer) +return ;blk_queue_io_min (sdkp->disk->queue,get_unaligned_be16 (&buffer[6 ]) * sector_sz);
@@ -1986,7 +1985,6 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)get_unaligned_be32 (&buffer[32 ]) & ~(1 <<31 ); } - out:kfree (buffer); }
@@ -1996,23 +1994,19 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)*/ static void sd_read_block_characteristics (struct scsi_disk *sdkp) { -unsigned char *buffer; +char *buffer;u16 rot; -const int vpd_len =32 ; - buffer =kmalloc (vpd_len,GFP_KERNEL ); - -if (!buffer || -/* Block Device Characteristics VPD */ -scsi_get_vpd_page (sdkp->device,0xb1 , buffer, vpd_len)) - goto out; +/* Block Device Characteristics VPD */ + buffer =scsi_get_vpd_page (sdkp->device,0xb1 ,64 ); +if (!buffer) +return ; rot =get_unaligned_be16 (&buffer[4 ]);if (rot =1 )queue_flag_set_unlocked (QUEUE_FLAG_NONROT , sdkp->disk->queue); - out:kfree (buffer); }
drivers/scsi/ses.c +3 -7
@@ -448,17 +448,13 @@ static void ses_match_to_enclosure(struct enclosure_device *edev, .addr = 0, }; - buf =kmalloc (INIT_ALLOC_SIZE ,GFP_KERNEL ); -if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf,INIT_ALLOC_SIZE )) - goto free; + buf =scsi_get_vpd_page (sdev,0x83 ,0 ); +if (!buf) +return ;ses_enclosure_data_process (edev,to_scsi_device (edev->edev.parent),0 ); vpd_len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; -kfree (buf); - buf =kmalloc (vpd_len,GFP_KERNEL ); -if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf, vpd_len)) - goto free; desc = buf +4 ;while (desc < buf + vpd_len) {
include/scsi/scsi_device.h +2 -2
@@ -348,8 +348,8 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp, struct scsi_sense_hdr *);extern int scsi_test_unit_ready (struct scsi_device *sdev,int timeout,int retries,struct scsi_sense_hdr *sshdr); -extern int scsi_get_vpd_page (struct scsi_device *,u8 page,unsigned char *buf, -int buf_len); +extern unsigned char *scsi_get_vpd_page (struct scsi_device *,u8 page, +unsigned len);extern int scsi_device_set_state (struct scsi_device *sdev,enum scsi_device_state state); extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
Signature · 8 lines
1.6.5.2 -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 6 lines · Martin K. Petersen
James,
While I agree the original VPD code's double
kmalloc()was a bit of a wart at least it did the right thing because it allocated a suitably sized buffer for page 0.
Quoted 6 lines
My concern with the interface you introduced in
e3deec09is that for devices that support a large number of VPD pages we won't be able to fit the page list in the allocated buffer. And callers are likely to pick a buffer size that makes sense for the VPD page they are interested in. It's not a big deal in the block limits/block device characteristics case because they are big enough.
Actually, I don't really think that's a problem. The design is to weed out USB devices that have silly returns, either by not returning a correct page zero, or by only accepting VPDs to the few pages and crashing on ones they don't support. If we run off the end of the buffer because there are 32 or more pages, then it's reasonably safe to assume the device will respond correctly to a VPD inquiry.
Quoted 3 lines
But at the very minimum that interface should come with a big fat warning in the comment section that describes that the page list must also be able to fit.
But it doesn't have to ... if we correctly return more pages than will fit in the buffer, it will proceed to do the requested page VPD inquiry.
James