Re: [PATCHv2] pci: Update VPD size with correct length

[Hannes Reinecke]

On Wednesday, December 16, 2015 09:13:35 AM Alexander Duyck wrote:
> On Wed, Dec 16, 2015 at 9:01 AM, Hannes Reinecke <hare@xxxxxxx> wrote:
> > On Wednesday, December 16, 2015 08:52:10 AM Alexander Duyck wrote:
> >> On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@xxxxxxx> wrote:
> >> > +               if (header[0] == 0xff) {
> >> > +                       /* Invalid data from VPD read */
> >> > +                       tag = header[0];
> >> > +               } else if (header[0] & 0x80) {
> >> > +                       /* Large Resource Data Type Tag */
> >> > +                       if (pci_read_vpd(dev, off+1, 2, &header[1]) !=
> >> > 2)
> >> > +                               return off + 1;
> >> > +                       off += 3 + ((header[2] << 8) | header[1]);
> >> > +                       tag = (header[0] & 0x7f);
> >> > +               } else {
> >> > +                       /* Short Resource Data Type Tag */
> >> > +                       off += 1 + (header[0] & 0x07);
> >> > +                       tag = (header[0] & 0x78) >> 3;
> >> > +               }
> >> > +               if (tag == 0x0f)        /* End tag descriptor */
> >> > +                       break;
> >> 
> >> It might make sense to just use the "return off" here since this is
> >> the only spot that should be returning the offset.
> > 
> > Which I'm not sure about.
> > We have three cases to worry about:
> > a) return after the 'end' tag
> > b) return after failing to read the 'end' tag
> > c) return after reading an invalid tag
> > 
> > For a) we obviously have to return the size.
> > But for b) and c)?
> > Just returning the maximal size (= old_size) would be exposing
> > invalid data to userland, with the possibility of hanging the system
> > by just reading from the attribute.
> > So to avoid that I've been returning the size of valid data.
> > 
> > But I'm open to suggestions if you think that's wrong.
> 
> If you didn't encounter an end tag how can you be sure you have valid
> data?  Maybe the random data managed to work out for the first couple
> of reads and then suddenly failed.  You might have a block of data
> that is valid for half of something like the read-only area and is
> going to return garbage data starting part way through.  I'd say you
> should handle this with an all-or-nothing type approach in order to
> err on the side of caution.  We could then see about white listing in
> those rare cases where a tag is missing using something like PCI quirk
> since we likely cannot use a parsing based approach if we cannot find
> the end tag.

Fair enough.

The only 'error' cases I've encountered so far is a read of all zeroes (and a 
halting the machine once you've read beyond a certain point) or a read of 0xff 
throughout the entire area. So that approach would work for both of them.

I'll be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		 zSeries & Storage
hare@xxxxxxx		+49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/