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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
