From: Hannes Reinecke <hare@suse.de>
To: Bjorn Helgaas <helgaas@kernel.org>,
Josselin Mouette <josselin.mouette@exaion.com>
Cc: linux-pci@vger.kernel.org
Subject: Re: [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"
Date: Fri, 3 May 2024 08:45:13 +0200 [thread overview]
Message-ID: <377ccedd-16e1-442d-9c1b-7ec18cf21fb5@suse.de> (raw)
In-Reply-To: <20240502222315.GA1551408@bhelgaas>
On 5/3/24 00:23, Bjorn Helgaas wrote:
> On Thu, Mar 07, 2024 at 04:36:06PM -0600, Bjorn Helgaas wrote:
>> [+cc Hannes, who did a lot of related VPD work and reviewed the
>> original posting at
>> https://lore.kernel.org/r/20210715215959.2014576-6-helgaas@kernel.org/]
>>
>> On Thu, Mar 07, 2024 at 05:09:27PM +0100, Josselin Mouette wrote:
>>> When a device returns invalid VPD data, it can be misused by other
>>> code paths in kernel space or user space, and there are instances
>>> in which this seems to cause memory corruption.
>>
>> More of the background from Josselin at
>> https://lore.kernel.org/r/aaea0b30c35bb73b947727e4b3ec354d6b5c399c.camel@exaion.com
>>
>> This is a regression, and obviously needs to be fixed somehow, but I'm
>> a bit hesitant to revert this until we understand the problem better.
>> If there's a memory corruption lurking and a revert hides the
>> corruption so we never fix it, I'm not sure that's an improvement
>> overall.
>
> I don't want to drop this, but we're kind of stuck here:
>
> - I'd still like to understand the problem better.
>
> - Trivially, I can't apply patches lacking the appropriate
> signed-off-by; see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.6#n396
>
Right. Again.
So: Problem is that VPD data is a self-describing stream of serialized
TLV records. The records themselves have no internal consistency check,
so any data read is assumed to be correct. If there is an error during
decoding we really should not continue, as everything beyond that error
cannot be relied on.
However, the assumption was that the read data is correct, so in getting
an error the very assumption leading us to interpret the data as VPD
records is invalid, too.
But question remains: was the data correct until the error?
Or was the error earlier, and we just mis-interpreted invalid data?
In my original patch I used the second attempt, as this will pretty
much guarantee that all VPD data will be correct. It has the drawback,
though, that for some cards we won't be able to read the VPD data, even
if they would provide valid data until the error.
As there is no good way out of this I'd rather see a blacklist of cards
which _can_ be read despite the error, and don't return VPD data otherwise.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
next prev parent reply other threads:[~2024-05-03 6:45 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-07 16:07 [Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found) Josselin Mouette
2024-03-07 16:09 ` [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid" Josselin Mouette
2024-03-07 16:10 ` [PATCH 2/2] Add better warnings about invalid VPD data Josselin Mouette
2024-03-07 22:36 ` [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid" Bjorn Helgaas
2024-05-02 22:23 ` Bjorn Helgaas
2024-05-03 6:45 ` Hannes Reinecke [this message]
2024-03-08 7:53 ` Josselin Mouette
2024-03-08 7:54 ` [PATCH 2/2] Add better warnings about invalid VPD data Josselin Mouette
2024-03-07 16:16 ` [Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found) Josselin Mouette
2024-03-07 23:11 ` Bjorn Helgaas
2024-03-08 7:42 ` Josselin Mouette
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=377ccedd-16e1-442d-9c1b-7ec18cf21fb5@suse.de \
--to=hare@suse.de \
--cc=helgaas@kernel.org \
--cc=josselin.mouette@exaion.com \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).