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

[Alexander Duyck]

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.
--
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/