login
Search
[PATCH] scsi: ses fix for len and mem leaking when fail to add intf
change to u32 before left shifting char
also fix leaking with scomp leaking when failing.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -369,7 +369,7 @@ static void ses_match_to_enclosure(struc
VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
goto free;
- len = (buf[2] << 8) + buf[3];
+ len = ((u32)buf[2] << 8) + buf[3];
desc = buf + 4;
while (desc < buf + len) {
enum scsi_protocol proto = desc[0] >> 4;
@@ -420,7 +420,7 @@ static int ses_intf_add(struct class_dev
if (!scsi_device_enclosure(sdev)) {
/* not an enclosure, but might be in one */
- edev = enclosure_find(&sdev->host->shost_gendev);
+ edev = enclosure_find(&sdev->host->shost_gendev);
if (edev) {
ses_match_to_enclosure(edev, sdev);
class_device_put(&edev->cdev);
@@ -451,18 +451,18 @@ static int ses_intf_add(struct class_dev
goto err_free;
}
- len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+ len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
buf = kzalloc(len, GFP_KERNEL);
if (!buf)
goto err_free;
- ses_dev->page1 = buf;
- ses_dev->page1_len = len;
-
result = ses_recv_diag(sdev, 1, buf, len);
if (result)
goto recv_failed;
+ ses_dev->page1 = buf;
+ ses_dev->page1_len = len;
+
types = buf[10];
len = buf[11];
@@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev
components += type_ptr[1];
}
+ buf = NULL;
result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
if (result)
goto recv_failed;
- len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+ len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
buf = kzalloc(len, GFP_KERNEL);
if (!buf)
goto err_free;
@@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev
/* The additional information page --- ...This one is a bit unnecessary; C promotion rules guarantee that everything is promoted to int (or above) before doing arithmetic. Since it's only ever done on 16 bits, signed or unsigned int is adequate for Yes, I see that, thanks! There's also the kmalloc of scomp which should Neither of us gets this right. By removing the kfree(buf) from the err_free path, you cause a leak here. I cause a double free. I think It's probably better to move these closer to the statements that make kfree(scomp) should be in the err_free path just in case someone else What exactly is this supposed to be doing, and why 7? If you're thinking of conditioning the page 7 receive on the success of the allocation, we really need the allocation failure report more than we James --
On Feb 9, 2008 7:00 AM, James Bottomley
thank. just learned that.
yhlu@yhlunb:~/xx/xx/notes> cat ctest.c
#include <stdio.h>
int main(int argc, char *argv[])
{
unsigned char buf[20];
int len;
buf[2] = 0x02;
buf[3] = 0x03;
len = (buf[2] << 8) + buf[3];
printf("len = %x\n", len);
return 0;
}
yhlu@yhlunb:~/xx/xx/notes> gcc -o ctest ctest.c
yhlu@yhlunb:~/xx/xx/notes> ./ctest
len = 203
the buf already become sdev->page1, sdev->pag10, sdev->page2.
--
[PATCH] scsi: ses fix mem leaking when fail to add intf
fix leaking with scomp leaking when failing.
also remove one extra space.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_dev
int i, j, types, len, components = 0;
int err = -ENOMEM;
struct enclosure_device *edev;
- struct ses_component *scomp;
+ struct ses_component *scomp = NULL;
if (!scsi_device_enclosure(sdev)) {
/* not an enclosure, but might be in one */
- edev = enclosure_find(&sdev->host->shost_gendev);
+ edev = enclosure_find(&sdev->host->shost_gendev);
if (edev) {
ses_match_to_enclosure(edev, sdev);
class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_dev
if (!buf)
goto err_free;
- ses_dev->page1 = buf;
- ses_dev->page1_len = len;
-
result = ses_recv_diag(sdev, 1, buf, len);
if (result)
goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_dev
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
components += type_ptr[1];
}
+ ses_dev->page1 = buf;
+ ses_dev->page1_len = len;
+ buf = NULL;
result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_dev
goto recv_failed;
ses_dev->page2 = buf;
ses_dev->page2_len = len;
+ buf = NULL;
/* The additional information page --- allows us
* to match up the devices */
@@ -506,11 +507,26 @@ static int ses_intf_add(struct class_dev
goto recv_failed;
ses_dev->page10 = buf;
ses_dev->page10_len = len;
+ buf = NULL;
no_page10:
- scomp = kmalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
+
+ /* Page 7 for the descriptors is optional */
+ result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
+ if ...There are still a few extraneous code moves in this one. This is about
the correct minimal set, isn't it?
James
---
From: Yinghai Lu <Yinghai.Lu@Sun.COM>
Date: Sat, 9 Feb 2008 15:15:47 -0800
Subject: [SCSI] ses: fix memory leaks
fix leaking with scomp leaking when failing. Also free page10 on
driver removal and remove one extra space.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/scsi/ses.c | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 2a6e4f4..8abc4a9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_device *cdev,
int i, j, types, len, components = 0;
int err = -ENOMEM;
struct enclosure_device *edev;
- struct ses_component *scomp;
+ struct ses_component *scomp = NULL;
if (!scsi_device_enclosure(sdev)) {
/* not an enclosure, but might be in one */
- edev = enclosure_find(&sdev->host->shost_gendev);
+ edev = enclosure_find(&sdev->host->shost_gendev);
if (edev) {
ses_match_to_enclosure(edev, sdev);
class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_device *cdev,
if (!buf)
goto err_free;
- ses_dev->page1 = buf;
- ses_dev->page1_len = len;
-
result = ses_recv_diag(sdev, 1, buf, len);
if (result)
goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_device *cdev,
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
components += type_ptr[1];
}
+ ses_dev->page1 = buf;
+ ses_dev->page1_len = len;
+ buf = NULL;
result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_device *cdev,
goto recv_failed;
ses_dev->page2 = buf;
ses_dev->page2_len = len;
+ buf = NULL;
/* The additional information page --- allows us
...if buf allocation for page 7 get NULL... if put + if (!buf) + goto err_free; still not right, because still undo edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id, components, &ses_enclosure_callbacks); all just add + if (!buf) + goto simple_populate; there? YH --
please check it...
---
From: Yinghai Lu <yinghai.lu@sun.com>
[SCSI] ses: fix memory leaks
fix leaking with scomp leaking when failing. Also free page10 on
driver removal and remove one extra space.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/scsi/ses.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_dev
int i, j, types, len, components = 0;
int err = -ENOMEM;
struct enclosure_device *edev;
- struct ses_component *scomp;
+ struct ses_component *scomp = NULL;
if (!scsi_device_enclosure(sdev)) {
/* not an enclosure, but might be in one */
- edev = enclosure_find(&sdev->host->shost_gendev);
+ edev = enclosure_find(&sdev->host->shost_gendev);
if (edev) {
ses_match_to_enclosure(edev, sdev);
class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_dev
if (!buf)
goto err_free;
- ses_dev->page1 = buf;
- ses_dev->page1_len = len;
-
result = ses_recv_diag(sdev, 1, buf, len);
if (result)
goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_dev
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
components += type_ptr[1];
}
+ ses_dev->page1 = buf;
+ ses_dev->page1_len = len;
+ buf = NULL;
result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_dev
goto recv_failed;
ses_dev->page2 = buf;
ses_dev->page2_len = len;
+ buf = NULL;
/* The additional information page --- allows us
* to match up the devices */
@@ -506,11 +507,12 @@ static int ses_intf_add(struct class_dev
goto recv_failed;
ses_dev->page10 = ...This one looks perfect, thanks! James --
Well, nearly perfect. I corrected this typo:
+ if (!buf)
+ goto simple_polulate;
^^^^^^^^
Which a compile check before you submitted the patch would have picked
up ...
James
--
sorry for that. --
one system: initrd get courrupted:
RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found. Try passing init= option to kernel.
bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sun Feb 3 15:48:56 2008 -0600
[SCSI] ses: add new Enclosure ULD
changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
so keep desc_ptr on right position
4. also record page7 len, and double check if desc_ptr out of boundary before touch.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
#include <scsi/scsi_host.h>
struct ses_device {
- char *page1;
- char *page2;
- char *page10;
+ unsigned char *page1;
+ unsigned char *page2;
+ unsigned char *page10;
short page1_len;
short page2_len;
short page10_len;
@@ -67,7 +67,7 @@ static int ses_probe(struct device *dev)
static int ses_recv_diag(struct scsi_device *sdev, int page_code,
void *buf, int bufflen)
{
- char cmd[] = {
+ unsigned char cmd[] = {
RECEIVE_DIAGNOSTIC,
1, /* Set PCV bit */
page_code,
@@ -85,7 +85,7 @@ static int ses_send_diag(struct scsi_dev
{
u32 result;
- char cmd[] = {
+ unsigned char cmd[] = {
SEND_DIAGNOSTIC,
0x10, /* Set PF bit */
0,
@@ -104,13 +104,13 @@ static int ses_send_diag(struct scsi_dev
static int ...Everything looks fine, thanks, except this piece.
That
if (x)
goto next;
...
next:
Needs to be
if (!x) {
...
}
I've fixed it up below. (I suppose the same thing goes for the
no_page10: label as well).
James
---
From: Yinghai Lu <Yinghai.Lu@Sun.COM>
Date: Tue, 12 Feb 2008 23:10:22 -0800
Subject: [SCSI] ses: fix data corruption
one system: initrd get courrupted:
RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found. Try passing init= option to kernel.
bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sun Feb 3 15:48:56 2008 -0600
[SCSI] ses: add new Enclosure ULD
changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not
enclosure_component_device/raid. so keep desc_ptr on right
position
4. also record page7 len, and double check if desc_ptr out of boundary
before touch.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/scsi/ses.c | 75 ++++++++++++++++++++++++++++-----------------------
1 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index a57fed4..614879e 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
#include <scsi/scsi_host.h>
struct ses_device {
- char *page1;
- char *page2;
- char *page10;
+ unsigned char *page1;
+ unsigned char *page2;
+ unsigned char *page10;
short page1_len;
short page2_len;
short page10_len;
@@ -67,7 ...find other problems about sub_enclosure... will send you updated one. YH --
one system: initrd get courrupted:
RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found. Try passing init= option to kernel.
bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sun Feb 3 15:48:56 2008 -0600
[SCSI] ses: add new Enclosure ULD
changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
so keep desc_ptr on right position
4. record page7 len, and double check if desc_ptr out of boundary before touch.
5. fix typo in subenclosure checking: should use hdr_buf instead.
Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
#include <scsi/scsi_host.h>
struct ses_device {
- char *page1;
- char *page2;
- char *page10;
+ unsigned char *page1;
+ unsigned char *page2;
+ unsigned char *page10;
short page1_len;
short page2_len;
short page10_len;
@@ -67,7 +66,7 @@ static int ses_probe(struct device *dev)
static int ses_recv_diag(struct scsi_device *sdev, int page_code,
void *buf, int bufflen)
{
- char cmd[] = {
+ unsigned char cmd[] = {
RECEIVE_DIAGNOSTIC,
1, /* Set PCV bit */
page_code,
@@ -85,7 +84,7 @@ static int ses_send_diag(struct scsi_dev
{
u32 result;
- char cmd[] = {
+ unsigned char cmd[] = {
SEND_DIAGNOSTIC,
0x10, /* Set PF bit */
0,
@@ -104,13 +103,13 @@ static int ...OK, I added this with a fixup to eliminate the spurious goto
Thanks,
James
---
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index cbba012..a6d9669 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -561,16 +561,15 @@ static int ses_intf_add(struct class_device *cdev,
if (desc_ptr) {
if (desc_ptr >= buf + page7_len) {
desc_ptr = NULL;
- goto noname;
+ } else {
+ len = (desc_ptr[2] << 8) + desc_ptr[3];
+ desc_ptr += 4;
+ /* Add trailing zero - pushes into
+ * reserved space */
+ desc_ptr[len] = '\0';
+ name = desc_ptr;
}
- len = (desc_ptr[2] << 8) + desc_ptr[3];
- desc_ptr += 4;
- /* Add trailing zero - pushes into
- * reserved space */
- desc_ptr[len] = '\0';
- name = desc_ptr;
}
- noname:
if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
--
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown | [PATCH 2/2] Subject: natsemi: Allow users to disable workaround for DspCfg reset |
| Tony Breeds | [LGUEST] Look in object dir for .config |
git: | |
| Brian Downing | Re: Git in a Nutshell guide |
| John Benes | Re: master has some toys |
| Matthias Lederhofer | [PATCH 4/7] introduce GIT_WORK_TREE to specify the work tree |
| Alexander Sulfrian | [RFC/PATCH] RE: git calls SSH_ASKPASS even if DISPLAY is not set |
| Junio C Hamano | Re: Rss produced by git is not valid xml? |
| Linux Kernel Mailing List | iSeries: fix section mismatch in iseries_veth |
| Linux Kernel Mailing List | ixbge: remove TX lock and redo TX accounting. |
| Linux Kernel Mailing List | ixgbe: fix several counter register errata |
| Linux Kernel Mailing List | b43: fix build with CONFIG_SSB_PCIHOST=n |
| Linux Kernel Mailing List | 9p: block-based virtio client |