A recent change to the megaraid driver to fix some memset calls resulted
in overflowing the arrays being cleared and causing a system panic.
This patch fixes the problem by making sure that the arrays being
cleared are dimensioned to the correct size. The patch has been tested
on osdl's stp machines that have megaraid controllers.
===== drivers/scsi/megaraid.c 1.45 vs edited =====
--- 1.45/drivers/scsi/megaraid.c Sat May 17 14:09:51 2003
+++ edited/drivers/scsi/megaraid.c Tue Jun 3 07:25:23 2003
@@ -723,7 +723,7 @@
{
dma_addr_t prod_info_dma_handle;
mega_inquiry3 *inquiry3;
- u8 raw_mbox[16];
+ u8 raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int retval;
@@ -732,7 +732,7 @@
mbox = (mbox_t *)raw_mbox;
memset((void *)adapter->mega_buffer, 0, MEGA_BUFFER_SIZE);
- memset(mbox, 0, 16);
+ memset(mbox, 0, sizeof(*mbox));
/*
* Try to issue Inquiry3 command
@@ -2415,7 +2415,7 @@
{
adapter_t *adapter;
mbox_t *mbox;
- u_char raw_mbox[16];
+ u_char raw_mbox[sizeof(mbox_t)];
char buf[12] = { 0 };
adapter = (adapter_t *)host->hostdata;
@@ -2424,7 +2424,7 @@
printk(KERN_NOTICE "megaraid: being unloaded...");
/* Flush adapter cache */
- memset(mbox, 0, 16);
+ memset(mbox, 0, sizeof(*mbox));
raw_mbox[0] = FLUSH_ADAPTER;
irq_disable(adapter);
@@ -2434,7 +2434,7 @@
issue_scb_block(adapter, raw_mbox);
/* Flush disks cache */
- memset(mbox, 0, 16);
+ memset(mbox, 0, sizeof(*mbox));
raw_mbox[0] = FLUSH_SYSTEM;
/* Issue a blocking (interrupts disabled) command to the card */
@@ -3896,7 +3896,7 @@
{
adapter_t *adapter;
struct Scsi_Host *host;
- u8 raw_mbox[16];
+ u8 raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int i,j;
@@ -3912,7 +3912,7 @@
mbox = (mbox_t *)raw_mbox;
/* Flush adapter cache */
- memset(mbox, 0, 16);
+ memset(mbox, 0, sizeof(*mbox));
raw_mbox[0] = FLUSH_ADAPTER;
irq_disable(adapter);
@@ -3925,7 +3925,7 @@
issue_scb_block(adapter, raw_mbox);
/* Flush disks cache */
- memset(mbox, 0, 16);
+ memset(mbox, 0, sizeof(*mbox));
raw_mbox[0] = FLUSH_SYSTEM;
issue_scb_block(adapter, raw_mbox);
@@ -4658,7 +4658,7 @@
static int
mega_is_bios_enabled(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int ret;
@@ -4691,7 +4691,7 @@
static void
mega_enum_raid_scsi(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int i;
@@ -4746,7 +4746,7 @@
mega_get_boot_drv(adapter_t *adapter)
{
struct private_bios_data *prv_bios_data;
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
u16 cksum = 0;
u8 *cksum_p;
@@ -4812,7 +4812,7 @@
static int
mega_support_random_del(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int rval;
@@ -4841,7 +4841,7 @@
static int
mega_support_ext_cdb(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
int rval;
@@ -4959,7 +4959,7 @@
static void
mega_get_max_sgl(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
mbox = (mbox_t *)raw_mbox;
@@ -5004,7 +5004,7 @@
static int
mega_support_cluster(adapter_t *adapter)
{
- unsigned char raw_mbox[16];
+ unsigned char raw_mbox[sizeof(mbox_t)];
mbox_t *mbox;
mbox = (mbox_t *)raw_mbox;
--
Mark Haverkamp <[email protected]>
On Tue, 2003-06-03 at 10:29, Mark Haverkamp wrote:
> A recent change to the megaraid driver to fix some memset calls resulted
> in overflowing the arrays being cleared and causing a system panic.
> This patch fixes the problem by making sure that the arrays being
> cleared are dimensioned to the correct size. The patch has been tested
> on osdl's stp machines that have megaraid controllers.
This patch doesn't quite look like a fix to me: The megaraid mailboxes
are always >16 bytes *but* none of the setting commands is supposed to
touch any of the status parts (which begin at byte 15), so I don't see
how your patch would prevent a panic.
It also looks like the first fifteen (not sixteen) bytes are user data
and the remaining 51 are for data from the card.
It thus looks like this memcpy in both issue_scb() and issue_scb_block()
may be wrong
memcpy((char *)mbox, (char *)scb->raw_mbox, 16);
because it's overwriting the mbox->busy return.
Logically, it looks like the mbox_t should be split up into an mbox_out
(which is what all the routines want to set values in) and an mbox_in
which is where the status is returned.
James
On Thu, 2003-06-05 at 07:07, James Bottomley wrote:
> On Tue, 2003-06-03 at 10:29, Mark Haverkamp wrote:
> > A recent change to the megaraid driver to fix some memset calls resulted
> > in overflowing the arrays being cleared and causing a system panic.
> > This patch fixes the problem by making sure that the arrays being
> > cleared are dimensioned to the correct size. The patch has been tested
> > on osdl's stp machines that have megaraid controllers.
>
> This patch doesn't quite look like a fix to me: The megaraid mailboxes
> are always >16 bytes *but* none of the setting commands is supposed to
> touch any of the status parts (which begin at byte 15), so I don't see
> how your patch would prevent a panic.
In the memset cases, what fixed the panic was that the size of the
raw_mbox automatic was set to 16 and the memset was using
sizeof(mbox_t). I just increased the size of the raw_mbox so it
wouldn't be overflowed. It sounds like, from what you are saying, that
the size of raw_mbox should have been left at 16 and the memset changed
to fill 16 bytes and not the sizeof(mbox_t).
>
> It also looks like the first fifteen (not sixteen) bytes are user data
> and the remaining 51 are for data from the card.
>
> It thus looks like this memcpy in both issue_scb() and issue_scb_block()
> may be wrong
>
> memcpy((char *)mbox, (char *)scb->raw_mbox, 16);
>
> because it's overwriting the mbox->busy return.
This doesn't seem like it would hurt since issue_scb sets mbox->busy
just after the memcpy. and in issue_scb_block, the raw_mbox busy
location is set before the memcpy.
>
> Logically, it looks like the mbox_t should be split up into an mbox_out
> (which is what all the routines want to set values in) and an mbox_in
> which is where the status is returned.
>
> James
--
Mark Haverkamp <[email protected]>
On Thu, 2003-06-05 at 10:33, Mark Haverkamp wrote:
> On Thu, 2003-06-05 at 07:07, James Bottomley wrote:
> > On Tue, 2003-06-03 at 10:29, Mark Haverkamp wrote:
> > > A recent change to the megaraid driver to fix some memset calls resulted
> > > in overflowing the arrays being cleared and causing a system panic.
> > > This patch fixes the problem by making sure that the arrays being
> > > cleared are dimensioned to the correct size. The patch has been tested
> > > on osdl's stp machines that have megaraid controllers.
> >
> > This patch doesn't quite look like a fix to me: The megaraid mailboxes
> > are always >16 bytes *but* none of the setting commands is supposed to
> > touch any of the status parts (which begin at byte 15), so I don't see
> > how your patch would prevent a panic.
>
> In the memset cases, what fixed the panic was that the size of the
> raw_mbox automatic was set to 16 and the memset was using
> sizeof(mbox_t). I just increased the size of the raw_mbox so it
> wouldn't be overflowed. It sounds like, from what you are saying, that
> the size of raw_mbox should have been left at 16 and the memset changed
> to fill 16 bytes and not the sizeof(mbox_t).
Ah, that's what I couldn't find in the source, thanks.
My observation is that only the first 15 bytes of mbox may be altered by
the user thus, since the issue_scb.. functions copy the mbox anyway,
there's not much point allocating the full mbox (although there's no
harm in doing so). But rather than going back to the 16 byte
allocations and fixing the memset sizes, I think mbox_t should be split
into two pieces (and out and an in, with the issue_scb..() routines only
taking the in part) that way everything can be correctly written in
terms of sizeof.
I was also separately worried about the memcpy in the issue_scb..()
routines which looks like it will set the mbox->busy parameter
(controlled by the driver) to zero. So I copied Atul to see if this is
a genuine problem or not.
James
On Thu, 2003-06-05 at 07:42, James Bottomley wrote:
> On Thu, 2003-06-05 at 10:33, Mark Haverkamp wrote:
> > On Thu, 2003-06-05 at 07:07, James Bottomley wrote:
> > > On Tue, 2003-06-03 at 10:29, Mark Haverkamp wrote:
> > > > A recent change to the megaraid driver to fix some memset calls resulted
> > > > in overflowing the arrays being cleared and causing a system panic.
> > > > This patch fixes the problem by making sure that the arrays being
> > > > cleared are dimensioned to the correct size. The patch has been tested
> > > > on osdl's stp machines that have megaraid controllers.
> > >
> > > This patch doesn't quite look like a fix to me: The megaraid mailboxes
> > > are always >16 bytes *but* none of the setting commands is supposed to
> > > touch any of the status parts (which begin at byte 15), so I don't see
> > > how your patch would prevent a panic.
> >
> > In the memset cases, what fixed the panic was that the size of the
> > raw_mbox automatic was set to 16 and the memset was using
> > sizeof(mbox_t). I just increased the size of the raw_mbox so it
> > wouldn't be overflowed. It sounds like, from what you are saying, that
> > the size of raw_mbox should have been left at 16 and the memset changed
> > to fill 16 bytes and not the sizeof(mbox_t).
>
> Ah, that's what I couldn't find in the source, thanks.
>
> My observation is that only the first 15 bytes of mbox may be altered by
> the user thus, since the issue_scb.. functions copy the mbox anyway,
> there's not much point allocating the full mbox (although there's no
> harm in doing so). But rather than going back to the 16 byte
> allocations and fixing the memset sizes, I think mbox_t should be split
> into two pieces (and out and an in, with the issue_scb..() routines only
> taking the in part) that way everything can be correctly written in
> terms of sizeof.
>
> I was also separately worried about the memcpy in the issue_scb..()
> routines which looks like it will set the mbox->busy parameter
> (controlled by the driver) to zero. So I copied Atul to see if this is
> a genuine problem or not.
The issue_scb.. functions set the busy parameter to 1 so the memcpy of
16 should be OK. For instance in issue_scb_block, busy is preset in the
raw_mbox before the memcpy.
Mark.
>
> James
--
Mark Haverkamp <[email protected]>
> > In the memset cases, what fixed the panic was that the size of the
> > raw_mbox automatic was set to 16 and the memset was using
> > sizeof(mbox_t). I just increased the size of the raw_mbox so it
> > wouldn't be overflowed. It sounds like, from what you are
> saying, that
> > the size of raw_mbox should have been left at 16 and the
> memset changed
> > to fill 16 bytes and not the sizeof(mbox_t).
>
> Ah, that's what I couldn't find in the source, thanks.
This is correct. Driver sets up first 16 bytes and firmware sets up rest.
Busy bit is special, driver sets it and firmware clears it.
> I was also separately worried about the memcpy in the issue_scb..()
> routines which looks like it will set the mbox->busy parameter
> (controlled by the driver) to zero. So I copied Atul to see
> if this is
> a genuine problem or not.
This is ok. Driver has to set it to busy anyway.
Coming back to main issue, declaring complete mailbox would be superfluous
since driver uses 16 bytes at most. The following patch should fix the panic
--- /usr/src/linux-2.5.70/drivers/scsi/megaraid.c 2003-05-26
21:00:20.000000000 -0400
+++ megaraid.c 2003-06-06 09:14:24.000000000 -0400
@@ -4664,7 +4664,7 @@
mbox = (mbox_t *)raw_mbox;
- memset(mbox, 0, sizeof(*mbox));
+ memset(mbox, 0, 16);
memset((void *)adapter->mega_buffer, 0, MEGA_BUFFER_SIZE);
@@ -4697,7 +4697,7 @@
mbox = (mbox_t *)raw_mbox;
- memset(mbox, 0, sizeof(*mbox));
+ memset(mbox, 0, 16);
/*
* issue command to find out what channels are raid/scsi
@@ -4813,12 +4813,9 @@
mega_support_random_del(adapter_t *adapter)
{
unsigned char raw_mbox[16];
- mbox_t *mbox;
int rval;
- mbox = (mbox_t *)raw_mbox;
-
- memset(mbox, 0, sizeof(*mbox));
+ memset(raw_mbox, 0, 16);
/*
* issue command
@@ -4842,12 +4839,9 @@
mega_support_ext_cdb(adapter_t *adapter)
{
unsigned char raw_mbox[16];
- mbox_t *mbox;
int rval;
- mbox = (mbox_t *)raw_mbox;
-
- memset(mbox, 0, sizeof(*mbox));
+ memset(raw_mbox, 0, 16);
/*
* issue command to find out if controller supports extended CDBs.
*/
On Fri, 2003-06-06 at 09:28, Mukker, Atul wrote:
> Coming back to main issue, declaring complete mailbox would be superfluous
> since driver uses 16 bytes at most. The following patch should fix the panic
>
> mbox = (mbox_t *)raw_mbox;
>
> - memset(mbox, 0, sizeof(*mbox));
> + memset(mbox, 0, 16);
>
> memset((void *)adapter->mega_buffer, 0, MEGA_BUFFER_SIZE);
>
This, I think, is a bad idea. It looks intrinsically wrong to allocate
storage and assign a pointer to it of a type that is longer than the
allocated storage. The initial buffer overrun was due to problems with
this.
I think the correct solution is to define your mailbox like this:
typedef struct {
/* 0x0 */ u8 cmd;
/* 0x1 */ u8 cmdid;
/* 0x2 */ u16 numsectors;
/* 0x4 */ u32 lba;
/* 0x8 */ u32 xferaddr;
/* 0xC */ u8 logdrv;
/* 0xD */ u8 numsgelements;
/* 0xE */ u8 resvd;
/* 0xF */ volatile u8 busy;
} __attribute__ ((packed)) user_mbox_t;
typedef struct {
user_mbox_t mbox_out
/* 0x10 */ volatile u8 numstatus;
/* 0x11 */ volatile u8 status;
/* 0x12 */ volatile u8 completed[MAX_FIRMWARE_STATUS];
volatile u8 poll;
volatile u8 ack;
} __attribute__ ((packed)) mbox_t;
and then re-define the issue_scb..() routines to use user_mbox_t which
is always the correct size.
Thus, you can throw away the raw_mbox and just do
user_mbox_t mbox;
memset(&mbox, 0, sizeof(mbox));
of course, your ->busy references become ->mbox_out.busy.
James
On Fri, 2003-06-06 at 06:46, James Bottomley wrote:
> On Fri, 2003-06-06 at 09:28, Mukker, Atul wrote:
> > Coming back to main issue, declaring complete mailbox would be superfluous
> > since driver uses 16 bytes at most. The following patch should fix the panic
> >
> > mbox = (mbox_t *)raw_mbox;
> >
> > - memset(mbox, 0, sizeof(*mbox));
> > + memset(mbox, 0, 16);
> >
> > memset((void *)adapter->mega_buffer, 0, MEGA_BUFFER_SIZE);
> >
>
> This, I think, is a bad idea. It looks intrinsically wrong to allocate
> storage and assign a pointer to it of a type that is longer than the
> allocated storage. The initial buffer overrun was due to problems with
> this.
>
> I think the correct solution is to define your mailbox like this:
>
> typedef struct {
> /* 0x0 */ u8 cmd;
> /* 0x1 */ u8 cmdid;
> /* 0x2 */ u16 numsectors;
> /* 0x4 */ u32 lba;
> /* 0x8 */ u32 xferaddr;
> /* 0xC */ u8 logdrv;
> /* 0xD */ u8 numsgelements;
> /* 0xE */ u8 resvd;
> /* 0xF */ volatile u8 busy;
> } __attribute__ ((packed)) user_mbox_t;
>
> typedef struct {
> user_mbox_t mbox_out
> /* 0x10 */ volatile u8 numstatus;
> /* 0x11 */ volatile u8 status;
> /* 0x12 */ volatile u8 completed[MAX_FIRMWARE_STATUS];
> volatile u8 poll;
> volatile u8 ack;
> } __attribute__ ((packed)) mbox_t;
>
> and then re-define the issue_scb..() routines to use user_mbox_t which
> is always the correct size.
>
> Thus, you can throw away the raw_mbox and just do
>
> user_mbox_t mbox;
> memset(&mbox, 0, sizeof(mbox));
>
> of course, your ->busy references become ->mbox_out.busy.
I started working on an update along these lines yesterday. I will test
it out today.
Mark.
--
Mark Haverkamp <[email protected]>
> > Coming back to main issue, declaring complete mailbox would
> be superfluous
> > since driver uses 16 bytes at most. The following patch
> should fix the panic
> >
> > mbox = (mbox_t *)raw_mbox;
> >
> > - memset(mbox, 0, sizeof(*mbox));
> > + memset(mbox, 0, 16);
> >
> > memset((void *)adapter->mega_buffer, 0, MEGA_BUFFER_SIZE);
> >
>
> This, I think, is a bad idea. It looks intrinsically wrong
> to allocate
> storage and assign a pointer to it of a type that is longer than the
> allocated storage. The initial buffer overrun was due to
> problems with
> this.
IMO then, your original patch should be good.