Message-ID: <2025022820-briskly-museum-c14d@gregkh>
Date: Fri, 28 Feb 2025 20:33:17 -0800
From: Greg KH <gregkh@...uxfoundation.org>
To: Alistair Francis <alistair23@...il.com>
Cc: Lukas Wunner <lukas@...ner.de>,
	Alistair Francis <alistair@...stair23.me>,
	linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, bhelgaas@...gle.com,
	Jonathan.Cameron@...wei.com, rust-for-linux@...r.kernel.org,
	akpm@...ux-foundation.org, boqun.feng@...il.com,
	bjorn3_gh@...tonmail.com, wilfred.mallawa@....com,
	aliceryhl@...gle.com, ojeda@...nel.org, a.hindborg@...nel.org,
	tmgross@...ch.edu, gary@...yguo.net, alex.gaynor@...il.com,
	benno.lossin@...ton.me, Alistair Francis <alistair.francis@....com>
Subject: Re: [RFC v2 09/20] PCI/CMA: Expose in sysfs whether devices are
 authenticated

On Fri, Feb 28, 2025 at 12:55:30PM +1000, Alistair Francis wrote:
> On Fri, Feb 28, 2025 at 11:41 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> >
> > On Thu, Feb 27, 2025 at 11:42:46PM +0100, Lukas Wunner wrote:
> > > On Thu, Feb 27, 2025 at 03:16:40AM -0800, Greg KH wrote:
> > > > On Thu, Feb 27, 2025 at 01:09:41PM +1000, Alistair Francis wrote:
> > > > > The PCI core has just been amended to authenticate CMA-capable devices
> > > > > on enumeration and store the result in an "authenticated" bit in struct
> > > > > pci_dev->spdm_state.
> > > > >
> > > > > Expose the bit to user space through an eponymous sysfs attribute.
> > > > >
> > > > > Allow user space to trigger reauthentication (e.g. after it has updated
> > > > > the CMA keyring) by writing to the sysfs attribute.
> > > > >
> > > > > Implement the attribute in the SPDM library so that other bus types
> > > > > besides PCI may take advantage of it.  They just need to add
> > > > > spdm_attr_group to the attribute groups of their devices and amend the
> > > > > dev_to_spdm_state() helper which retrieves the spdm_state for a given
> > > > > device.
> > > > >
> > > > > The helper may return an ERR_PTR if it couldn't be determined whether
> > > > > SPDM is supported by the device.  The sysfs attribute is visible in that
> > > > > case but returns an error on access.  This prevents downgrade attacks
> > > > > where an attacker disturbs memory allocation or DOE communication
> > > > > in order to create the appearance that SPDM is unsupported.
> > > >
> > > > I don't like this "if it's present we still don't know if the device
> > > > supports this", as that is not normally the "sysfs way" here.  Why must
> > > > it be present in those situations?
> 
> I do think there are 4 situations
> 
>  1. Device supports authentication and was authenticated
>  2.
>   a) Device supports authentication, but the certificate chain can't
> be authenticated
>   b) Device supports authentication, but the communication was interrupted
>  3. Device doesn't support authentication
> 
> Case 1 and 3 are easy
> 
> Case 2 (a) means that the kernel doesn't trust the certificate. This
> could be for a range of reasons, including the user just hasn't
> installed the cert yet. So it makes sense to pass that information to
> the user as they can act on it. I think it's a different case to 3
> (where there is no action for the user to take).

Ok, that's fine, but as a "default" you should fail such that the device
is not allowed to actually do anything if 2) happens.  I think you are
doing that, it's the whole use of sysfs as the user/kernel api here in
an odd way that I am objecting to.

> > Again, are we now claiming that Linux needs to support "hardware
> > glitching"?  Is that required somewhere?  I think if the DOE exchanges
> > fail, we just trust the device as we have to trust something, right?
> 
> This is case 2 (b) above. If an attacker could flip a bit or drop a
> byte in the communication path they could cause the authentication to
> stop.
> 
> The process to authenticate a device involves sending a lot of data,
> so it's not impossible that an attacker could interrupt some of the
> traffic.
> 
> If the DOE exchange fails I don't think we want to trust the device,
> as that's the point of SPDM. So being able to communicate that to
> userspace does seem really useful.

Agreed, but again, let's come up with a better api than "a random sysfs
file is present but reading it gets an error to convey this is an
unvalidated device" feels wrong to me.

Why not just report the "state" of the device?  You have 3 states, just
report that?  Should be much simpler overall.

> > > Of course, this is an abnormal situation that users won't encounter
> > > unless they're being attacked.  Normally the attribute is only present
> > > if authentication is supported.
> > >
> > > I disagree with your assessment that we have bigger problems.
> > > For security protocols like this we have to be very careful
> > > to prevent trivial circumvention.  We cannot just shrug this off
> > > as unimportant.
> >
> > hardware glitching is not trivial.  Let's only worry about that if the
> > hardware people somehow require it, and if so, we can push back and say
> > "stop making us fix your broken designs" :)
> 
> Hardware glitching is hard to do, but SPDM is designed to protect
> against a range of hard attacks. The types of attacks that nation
> states would do. So I think we should do our best to protect against
> them.Obviously there is only so much that software can do to protect
> against a hardware glitch, but this seems like a good in between.
> 
> I don't think the design is overall broken though. At some point when
> all devices support SPDM it becomes a non issue as you just fail if
> authentication fails, but in the mean time we need to handle devices
> with and without authentication.

Agreed, again it's the user/kernel api I am objecting to here the most.

> > > The "authenticated" attribute tells user space whether the device
> > > is authenticated.  User space needs to handle errors anyway when
> > > reading the attribute.  Users will get an error if authentication
> > > support could not be determined.  Simple.
> >
> > No, if it's not determined, it shouldn't be present.
> 
> That doesn't allow us to differentiate the cases I mentioned above though.
> 
> Another option is we could create multiple attributes "spdm",
> "authenticated" and "spdm_err" for example to convey the information
> with just yes/no attributes?

Yes!  Or a simple "spdm_state" file that has one of these values in it?
sysfs files do not need to only return "1 or 0", they can return
different strings, but only 1 at a time.

thanks,

greg k-h

Powered by blists - more mailing lists