From: Jesper Juhl <[email protected]>
in sas_get_phy_change_count(), the line
disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
#define DISCOVER_RESP_SIZE 56
But, the struct is actually 60 bytes in size.
So change the define to be
#define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
so we always get the correct size even when people
fiddle with the structure.
This change also fixes the same problem in
sas_get_phy_attached_sas_addr()
(Found by the Coverity checker. Compile tested only)
Signed-off-by: Jesper Juhl <[email protected]>
---
sas_expander.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 8727436..a666cb1 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -211,7 +211,7 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
}
#define DISCOVER_REQ_SIZE 16
-#define DISCOVER_RESP_SIZE 56
+#define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
u8 *disc_resp, int single)
On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
> From: Jesper Juhl <[email protected]>
>
> in sas_get_phy_change_count(), the line
> disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> will allocate 56 bytes due to this define:
> #define DISCOVER_RESP_SIZE 56
> But, the struct is actually 60 bytes in size.
>
> So change the define to be
> #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
> so we always get the correct size even when people
> fiddle with the structure.
>
> This change also fixes the same problem in
> sas_get_phy_attached_sas_addr()
>
> (Found by the Coverity checker. Compile tested only)
Well, your fix is definitely wrong.
Could you explain the problem a little more? The discover response SMP
frame is 56 bytes as mandated by the standard. I don't see anywhere in
the code where we're actually using a value beyond the 56th byte ...
where is the problem use?
James
On 12/11/2007, James Bottomley <[email protected]> wrote:
> On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
> > From: Jesper Juhl <[email protected]>
> >
> > in sas_get_phy_change_count(), the line
> > disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> > will allocate 56 bytes due to this define:
> > #define DISCOVER_RESP_SIZE 56
> > But, the struct is actually 60 bytes in size.
> >
> > So change the define to be
> > #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
> > so we always get the correct size even when people
> > fiddle with the structure.
> >
> > This change also fixes the same problem in
> > sas_get_phy_attached_sas_addr()
> >
> > (Found by the Coverity checker. Compile tested only)
>
> Well, your fix is definitely wrong.
>
> Could you explain the problem a little more? The discover response SMP
> frame is 56 bytes as mandated by the standard. I don't see anywhere in
> the code where we're actually using a value beyond the 56th byte ...
> where is the problem use?
>
I haven't found any actual problem *use*, I just looked at the size of
'struct smp_resp' and noticed that coverity seemed to be right that 56
bytes are not sufficient to hold the members of the struct. There are
32 bytes in the first members + the union and I don't see how that can
ever stay at 56 bytes...? So, we are allocating memory and storing it
in a 'struct smp_resp *', but we are allocating less than
sizeof(smp_resp) - how is that not a (potential) problem?
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
James Bottomley wrote:
> On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
>> From: Jesper Juhl <[email protected]>
>>
>> in sas_get_phy_change_count(), the line
>> disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
>> will allocate 56 bytes due to this define:
>> #define DISCOVER_RESP_SIZE 56
>> But, the struct is actually 60 bytes in size.
>>
>> So change the define to be
>> #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
>> so we always get the correct size even when people
>> fiddle with the structure.
>>
>> This change also fixes the same problem in
>> sas_get_phy_attached_sas_addr()
>>
>> (Found by the Coverity checker. Compile tested only)
>
> Well, your fix is definitely wrong.
>
> Could you explain the problem a little more? The discover response SMP
> frame is 56 bytes as mandated by the standard. I don't see anywhere in
> the code where we're actually using a value beyond the 56th byte ...
> where is the problem use?
The response size of SMP DISCOVER keeps growing with
each rev. Currently in SAS-2 revision 12 it is 112 bytes long!
The original SAS standard and SAS 1.1 have implicit response
sizes and for DISCOVER that is 56 bytes.
To be compliant with SAS-2 the code should read byte 3 of
a DISCOVER response. If it is zero then the response length
is 56 bytes, otherwise the response length is that many
dwords (i.e. 4 byte units) plus 4 bytes for the CRC.
[Similar logic applies to many other SMP responses.]
I have tables for all SMP requests and response (up until
SAS-2 rev 12) in my smp_utils package, see the smp_lib.c
file.
Doug Gilbert
On Mon, 2007-11-12 at 01:13 +0100, Jesper Juhl wrote:
> On 12/11/2007, James Bottomley <[email protected]> wrote:
> > On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
> > > From: Jesper Juhl <[email protected]>
> > >
> > > in sas_get_phy_change_count(), the line
> > > disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> > > will allocate 56 bytes due to this define:
> > > #define DISCOVER_RESP_SIZE 56
> > > But, the struct is actually 60 bytes in size.
> > >
> > > So change the define to be
> > > #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
> > > so we always get the correct size even when people
> > > fiddle with the structure.
> > >
> > > This change also fixes the same problem in
> > > sas_get_phy_attached_sas_addr()
> > >
> > > (Found by the Coverity checker. Compile tested only)
> >
> > Well, your fix is definitely wrong.
> >
> > Could you explain the problem a little more? The discover response SMP
> > frame is 56 bytes as mandated by the standard. I don't see anywhere in
> > the code where we're actually using a value beyond the 56th byte ...
> > where is the problem use?
> >
>
> I haven't found any actual problem *use*, I just looked at the size of
> 'struct smp_resp' and noticed that coverity seemed to be right that 56
> bytes are not sufficient to hold the members of the struct.
There are 11 current (well, as of the 1.1 standard) SMP frame responses.
Each of them is actually a different length. This driver treats SMP
frame response underruns as errors, so the fix you propose would cause
the whole discovery process to collapse.
> There are
> 32 bytes in the first members + the union and I don't see how that can
> ever stay at 56 bytes...? So, we are allocating memory and storing it
> in a 'struct smp_resp *', but we are allocating less than
> sizeof(smp_resp) - how is that not a (potential) problem?
We're not storing anything. We're allocating a byte area for the driver
to deposit a response frame, we know we just sent a SMP DISCOVER
request, so we'd better get a discover response back (and the driver
will error on underrun or overrun, so we know if it returns successfully
that's exatly what we have). The struct smp_resp contains the SMP
invariant header and then a union of all the other response data types,
so as long as we use either the header or the disc piece of the union,
there's no possibility of error, is there?
James
On Sun, 2007-11-11 at 19:33 -0500, Douglas Gilbert wrote:
> James Bottomley wrote:
> > On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
> >> From: Jesper Juhl <[email protected]>
> >>
> >> in sas_get_phy_change_count(), the line
> >> disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> >> will allocate 56 bytes due to this define:
> >> #define DISCOVER_RESP_SIZE 56
> >> But, the struct is actually 60 bytes in size.
> >>
> >> So change the define to be
> >> #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
> >> so we always get the correct size even when people
> >> fiddle with the structure.
> >>
> >> This change also fixes the same problem in
> >> sas_get_phy_attached_sas_addr()
> >>
> >> (Found by the Coverity checker. Compile tested only)
> >
> > Well, your fix is definitely wrong.
> >
> > Could you explain the problem a little more? The discover response SMP
> > frame is 56 bytes as mandated by the standard. I don't see anywhere in
> > the code where we're actually using a value beyond the 56th byte ...
> > where is the problem use?
>
> The response size of SMP DISCOVER keeps growing with
> each rev. Currently in SAS-2 revision 12 it is 112 bytes long!
> The original SAS standard and SAS 1.1 have implicit response
> sizes and for DISCOVER that is 56 bytes.
>
> To be compliant with SAS-2 the code should read byte 3 of
> a DISCOVER response. If it is zero then the response length
> is 56 bytes, otherwise the response length is that many
> dwords (i.e. 4 byte units) plus 4 bytes for the CRC.
> [Similar logic applies to many other SMP responses.]
>
> I have tables for all SMP requests and response (up until
> SAS-2 rev 12) in my smp_utils package, see the smp_lib.c
> file.
Right at the moment, we're requiring the 1.1 56 byte response because
the request has byte three zeroed (sas 2 requires responders to return
the 1.1 response frame in this case). I suppose if we ever need to do
zoning, there might be a case to move to larger sizes, but at the moment
it would just tell us about features we're not using anyway (plus we're
bound to confuse some older expanders).
James
On 12/11/2007, James Bottomley <[email protected]> wrote:
> On Mon, 2007-11-12 at 01:13 +0100, Jesper Juhl wrote:
> > On 12/11/2007, James Bottomley <[email protected]> wrote:
> > > On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote:
> > > > From: Jesper Juhl <[email protected]>
> > > >
> > > > in sas_get_phy_change_count(), the line
> > > > disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> > > > will allocate 56 bytes due to this define:
> > > > #define DISCOVER_RESP_SIZE 56
> > > > But, the struct is actually 60 bytes in size.
> > > >
> > > > So change the define to be
> > > > #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)
> > > > so we always get the correct size even when people
> > > > fiddle with the structure.
> > > >
> > > > This change also fixes the same problem in
> > > > sas_get_phy_attached_sas_addr()
> > > >
> > > > (Found by the Coverity checker. Compile tested only)
> > >
> > > Well, your fix is definitely wrong.
> > >
> > > Could you explain the problem a little more? The discover response SMP
> > > frame is 56 bytes as mandated by the standard. I don't see anywhere in
> > > the code where we're actually using a value beyond the 56th byte ...
> > > where is the problem use?
> > >
> >
> > I haven't found any actual problem *use*, I just looked at the size of
> > 'struct smp_resp' and noticed that coverity seemed to be right that 56
> > bytes are not sufficient to hold the members of the struct.
>
> There are 11 current (well, as of the 1.1 standard) SMP frame responses.
> Each of them is actually a different length. This driver treats SMP
> frame response underruns as errors, so the fix you propose would cause
> the whole discovery process to collapse.
>
> > There are
> > 32 bytes in the first members + the union and I don't see how that can
> > ever stay at 56 bytes...? So, we are allocating memory and storing it
> > in a 'struct smp_resp *', but we are allocating less than
> > sizeof(smp_resp) - how is that not a (potential) problem?
>
> We're not storing anything. We're allocating a byte area for the driver
> to deposit a response frame, we know we just sent a SMP DISCOVER
> request, so we'd better get a discover response back (and the driver
> will error on underrun or overrun, so we know if it returns successfully
> that's exatly what we have). The struct smp_resp contains the SMP
> invariant header and then a union of all the other response data types,
> so as long as we use either the header or the disc piece of the union,
> there's no possibility of error, is there?
>
No, as long as the driver will err on underrun and overrun (of 56
bytes) and we only use those parts of the structure that lie within
the first 56 bytes, then I don't see how it could fail.
But it still makes the hairs on the back of my head stand on end to
see memory allocated, and pointed to by a "struct foo *", that is less
than "sizeof(struct foo)"...
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html