On 12/11/2007, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > On Mon, 2007-11-12 at 01:13 +0100, Jesper Juhl wrote: > > On 12/11/2007, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > On Mon, 2007-11-12 at 00:24 +0100, Jesper Juhl wrote: > > > > From: Jesper Juhl <jesper.juhl@xxxxxxxxx> > > > > > > > > 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 <jesper.juhl@xxxxxxxxx> 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-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
