Re: [PATCH v2 6/6] soc: qcom: socinfo: Convert to Rust

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Feb 03, 2026 at 09:37:50AM -0800, Matthew Maurer wrote:
> On Tue, Feb 3, 2026 at 8:28 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Feb 03, 2026 at 03:46:35PM +0000, Matthew Maurer wrote:
> > > Convert the socinfo driver to Rust for a number of improvements:
> > > * Accessing IO mapped regions through the IO subsystem, rather than
> > >   through regular memory accesses.
> >
> > That's good, but the C code could also be "fixed" to do this, right?
> 
> Yes, nothing stops the C code from being fixed to do all of this - all
> of this is representable in C code.

Great!  How about sending fixes for that first so that older kernels can
benifit from those fixes :)

> > > * Binds the device as an auxiliary device rather than a platform device,
> > >   ensuring the mapped IO regions cannot be accessed after the smem
> > >   device is removed.
> >
> > I'm all for this, but is this really an aux device?  What is the
> > "parent" device of this aux device?  Where are the "siblings"?  What
> > does sysfs look like before/after this?
> 
> The parent of this aux device is qcom-smem. In the prior
> implementation, qcom-smem loads this device through
> `platform_device_register_data` and `platform_device_unregister`,
> holding a reference in its global struct to release it when being
> released. The probe table for the qcom-socinfo driver was empty, so it
> would not probe without an explicit registration.

So it's a "fake" platform device being created?  As Bjorn said, moving
this to aux today would probably be good first.

> > > * Adds bounds-checking to all accesses, hardening against a repeat of
> > >   CVE-2024-58007
> >
> > How do you now "know" that the bounds checking is correct?  The C
> > version also had this, it was just "not correct" :)
> 
> While it's technically possible for the Rust code to have an error
> here, the error would not be in the driver, but in the kernel crate.
> The advantage here is that the bounds checking is all centralized, so
> we get it right once, for the entire kernel, instead of needing to get
> it right every time.

I missed where the bounds checking is happening at all here.  I see
fields and sizes of fields, but what is verifying that those sizes are
actually correct?

> > And which accesses are you referring to?  From userspace?  From the
> > kernel?  That CVE looks very odd, it's probably not even a real one and
> > should be revoked, right?
> >
> 
> That CVE looks like this:
> 1. qcom_smem_get returns an object of size N
> 2. When initializing the `serial_number` field of
> soc_device_attributes, the offset of the serial number field was
> checked as <= N, rather than the *end* of the serial number field.
> 3. This resulted in the driver exposing through sysfs whatever data
> was mapped afterwards, interpreted as a number.
> 
> I agree that the severity seems oddly high, given that in practice,
> this will expose the remainder of the IO mapped page - I don't believe
> it crosses a page boundary, so it can't even *really* leak anything.

Where do you see this as "high"?  The kernel CNA does NOT give any
severity info for any CVEs that are issued.

If you are looking at the crap that NVD adds, well, it's just that,
crap, that has no actual relevance to anyone and is flat out wrong most
of the time.  We have asked them to stop doing this, but so far we have
not gotten a response:
	https://github.com/cisagov/vulnrichment/issues/262

In short, for kernel CVEs only look at the data provided by cve.org, or
the raw data provided by the kernel CNA team.

That being said, this seems like a basic "we are just displaying wrong
data in a sysfs file", so it probably should be rejected unless someone
can tell me how it fits the definition of a vulnerability?

As for why it's a CVE at all, it came in as part of the required GSD
import into CVE.org.

> > > +void *qcom_smem_get_aux(struct auxiliary_device *aux, unsigned int host,
> > > +             unsigned int item, size_t *size)
> > > +{
> > > +     if (IS_ERR(__smem))
> > > +             return __smem;
> > > +     if (aux->dev.parent != __smem->dev)
> > > +             return ERR_PTR(-EINVAL);
> > > +     return qcom_smem_get(host, item, size);
> >
> > So you are returning a void pointer?  But don't you really know the
> > "type" of what is being asked here?  It's a memory chunk, so u8?  Or
> > something else?  void * feels "rough" here.
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_smem_get_aux);
> > > +
> > >  /**
> > >   * qcom_smem_get() - resolve ptr of size of a smem item
> > >   * @host:    the remote processor, or -1
> > > @@ -684,6 +710,9 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
> > >   * Looks up smem item and returns pointer to it. Size of smem
> > >   * item is returned in @size.
> > >   *
> > > + * It is up to the caller to ensure that the qcom_smem device remains
> > > + * loaded by some mechanism when accessing returned memory.
> >
> > What do you mean by "loaded"?  You are saying that the caller needs to
> > rely on this driver remaining in memory for that memory to be "valid"?
> >
> > If this is the case, why not take a reference count?
> >
> > > +impl Smem {
> > > +    pub(crate) fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Option<&'a Mmio> {
> > > +        if *dev != *self.dev {
> >
> > How can this ever happen?

I think you missed these review comments?

thanks,

greg k-h