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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
