From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Yinghai Lu <Yinghai.Lu@Sun.COM>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org,
kristen.c.accardi@intel.com, Ingo Molnar <mingo@elte.hu>,
"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH] SCSI: fix data corruption caused by ses
Date: Wed, 13 Feb 2008 17:25:27 -0600 [thread overview]
Message-ID: <1202945127.3109.89.camel@localhost.localdomain> (raw)
In-Reply-To: <200802122310.23050.yinghai.lu@sun.com>
On Tue, 2008-02-12 at 23:10 -0800, Yinghai Lu wrote:
> if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
> type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
> - continue;
> + goto next;
> +
> ecomp = enclosure_component_register(edev,
> components++,
> type_ptr[0],
> name);
> +
> + if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
> + ses_process_descriptor(ecomp,
> + addl_desc_ptr);
> + next:
> if (desc_ptr) {
> desc_ptr += len;
> - if (!IS_ERR(ecomp))
> - ses_process_descriptor(ecomp,
> - addl_desc_ptr);
>
> if (addl_desc_ptr)
> addl_desc_ptr += addl_desc_ptr[1] + 2;
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 +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_device *sdev, int page_code,
{
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_device *sdev, int page_code,
static int ses_set_page2_descriptor(struct enclosure_device *edev,
struct enclosure_component *ecomp,
- char *desc)
+ unsigned char *desc)
{
int i, j, count = 0, descriptor = ecomp->number;
struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
struct ses_device *ses_dev = edev->scratch;
- char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
- char *desc_ptr = ses_dev->page2 + 8;
+ unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+ unsigned char *desc_ptr = ses_dev->page2 + 8;
/* Clear everything */
memset(desc_ptr, 0, ses_dev->page2_len - 8);
@@ -133,14 +133,14 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
}
-static char *ses_get_page2_descriptor(struct enclosure_device *edev,
+static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
struct enclosure_component *ecomp)
{
int i, j, count = 0, descriptor = ecomp->number;
struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
struct ses_device *ses_dev = edev->scratch;
- char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
- char *desc_ptr = ses_dev->page2 + 8;
+ unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+ unsigned char *desc_ptr = ses_dev->page2 + 8;
ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
@@ -160,17 +160,18 @@ static char *ses_get_page2_descriptor(struct enclosure_device *edev,
static void ses_get_fault(struct enclosure_device *edev,
struct enclosure_component *ecomp)
{
- char *desc;
+ unsigned char *desc;
desc = ses_get_page2_descriptor(edev, ecomp);
- ecomp->fault = (desc[3] & 0x60) >> 4;
+ if (desc)
+ ecomp->fault = (desc[3] & 0x60) >> 4;
}
static int ses_set_fault(struct enclosure_device *edev,
struct enclosure_component *ecomp,
enum enclosure_component_setting val)
{
- char desc[4] = {0 };
+ unsigned char desc[4] = {0 };
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
@@ -190,26 +191,28 @@ static int ses_set_fault(struct enclosure_device *edev,
static void ses_get_status(struct enclosure_device *edev,
struct enclosure_component *ecomp)
{
- char *desc;
+ unsigned char *desc;
desc = ses_get_page2_descriptor(edev, ecomp);
- ecomp->status = (desc[0] & 0x0f);
+ if (desc)
+ ecomp->status = (desc[0] & 0x0f);
}
static void ses_get_locate(struct enclosure_device *edev,
struct enclosure_component *ecomp)
{
- char *desc;
+ unsigned char *desc;
desc = ses_get_page2_descriptor(edev, ecomp);
- ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
+ if (desc)
+ ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
}
static int ses_set_locate(struct enclosure_device *edev,
struct enclosure_component *ecomp,
enum enclosure_component_setting val)
{
- char desc[4] = {0 };
+ unsigned char desc[4] = {0 };
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
@@ -229,7 +232,7 @@ static int ses_set_active(struct enclosure_device *edev,
struct enclosure_component *ecomp,
enum enclosure_component_setting val)
{
- char desc[4] = {0 };
+ unsigned char desc[4] = {0 };
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
@@ -409,11 +412,11 @@ static int ses_intf_add(struct class_device *cdev,
{
struct scsi_device *sdev = to_scsi_device(cdev->dev);
struct scsi_device *tmp_sdev;
- unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr,
- *addl_desc_ptr;
+ unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr = NULL,
+ *addl_desc_ptr = NULL;
struct ses_device *ses_dev;
u32 result;
- int i, j, types, len, components = 0;
+ int i, j, types, len, page7_len = 0, components = 0;
int err = -ENOMEM;
struct enclosure_device *edev;
struct ses_component *scomp = NULL;
@@ -461,9 +464,8 @@ static int ses_intf_add(struct class_device *cdev,
goto recv_failed;
types = buf[10];
- len = buf[11];
- type_ptr = buf + 12 + len;
+ type_ptr = buf + 12 + buf[11];
for (i = 0; i < types; i++, type_ptr += 4) {
if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
@@ -530,7 +532,7 @@ static int ses_intf_add(struct class_device *cdev,
if (result)
goto simple_populate;
- len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+ page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
/* add 1 for trailing '\0' we'll use */
buf = kzalloc(len + 1, GFP_KERNEL);
if (!buf)
@@ -547,7 +549,8 @@ static int ses_intf_add(struct class_device *cdev,
len = (desc_ptr[2] << 8) + desc_ptr[3];
/* skip past overall descriptor */
desc_ptr += len + 4;
- addl_desc_ptr = ses_dev->page10 + 8;
+ if (ses_dev->page10)
+ addl_desc_ptr = ses_dev->page10 + 8;
}
type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
components = 0;
@@ -557,6 +560,8 @@ static int ses_intf_add(struct class_device *cdev,
struct enclosure_component *ecomp;
if (desc_ptr) {
+ if (desc_ptr >= buf + page7_len)
+ break;
len = (desc_ptr[2] << 8) + desc_ptr[3];
desc_ptr += 4;
/* Add trailing zero - pushes into
@@ -564,18 +569,20 @@ static int ses_intf_add(struct class_device *cdev,
desc_ptr[len] = '\0';
name = desc_ptr;
}
- if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
- type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
- continue;
- ecomp = enclosure_component_register(edev,
+ if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
+ type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
+
+ ecomp = enclosure_component_register(edev,
components++,
type_ptr[0],
name);
- if (desc_ptr) {
- desc_ptr += len;
- if (!IS_ERR(ecomp))
+
+ if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
ses_process_descriptor(ecomp,
addl_desc_ptr);
+ }
+ if (desc_ptr) {
+ desc_ptr += len;
if (addl_desc_ptr)
addl_desc_ptr += addl_desc_ptr[1] + 2;
--
1.5.3.8
next prev parent reply other threads:[~2008-02-13 23:25 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-02-09 12:13 [PATCH] scsi: ses fix for len and mem leaking when fail to add intf Yinghai Lu
2008-02-09 15:00 ` James Bottomley
2008-02-09 22:28 ` Yinghai Lu
2008-02-09 23:15 ` [PATCH] scsi: ses fix " Yinghai Lu
2008-02-11 4:28 ` James Bottomley
2008-02-11 5:27 ` Yinghai Lu
2008-02-11 7:25 ` [SCSI] ses: fix memory leaks Yinghai Lu
2008-02-11 16:23 ` James Bottomley
2008-02-11 17:02 ` James Bottomley
2008-02-11 20:25 ` Yinghai Lu
2008-02-13 7:10 ` [PATCH] SCSI: fix data corruption caused by ses Yinghai Lu
2008-02-13 23:25 ` James Bottomley [this message]
2008-02-14 0:07 ` Yinghai Lu
2008-02-14 0:25 ` [PATCH] SCSI: fix data corruption caused by ses v2 Yinghai Lu
2008-02-15 15:53 ` James Bottomley
2008-02-15 18:44 ` Yinghai Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1202945127.3109.89.camel@localhost.localdomain \
--to=james.bottomley@hansenpartnership.com \
--cc=Yinghai.Lu@Sun.COM \
--cc=akpm@linux-foundation.org \
--cc=hpa@zytor.com \
--cc=kristen.c.accardi@intel.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=mingo@elte.hu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.