linux-kernel - Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YkWj/vmjohNo0r2c@kuha.fi.intel.com>
Date:   Thu, 31 Mar 2022 15:52:14 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Takashi Iwai <tiwai@...e.de>, Won Chung <wonchung@...gle.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Benson Leung <bleung@...omium.org>,
        Prashant Malani <pmalani@...omium.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback
 function

Hi Greg,

On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
> On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
> > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > -     if (!strcmp(dev->driver->name, "i915") &&
> > > > > > +     if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > >
> > > > > Can NULL dev->driver be really seen?  I thought the components are
> > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > But there can be corner cases I overlooked.
> > > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Takashi
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > When I try using component_add in a different driver (usb4 in my
> > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > not have their component master fully bound when this new component is
> > > > registered. When I test it, it seems to be causing a crash.
> > > 
> > > Hm, from where component_add*() is called?  Basically dev->driver must
> > > be already set before the corresponding driver gets bound at
> > > __driver_probe_deviec().  So, if the device is added to component from
> > > the corresponding driver's probe, dev->driver must be non-NULL.
> > 
> > The code that declares a device as component does not have to be the
> > driver of that device.
> > 
> > In our case the components are USB ports, and they are devices that
> > are actually never bind to any drivers: drivers/usb/core/port.c
> 
> Why is a USB device being passed to this code that assumes it is looking
> for a PCI device with a specific driver name?  As I mentioned on the
> mei patch, triggering off of a name is really a bad idea, as is assuming
> the device type without any assurance it is such a device (there's a
> reason we didn't provide device type identification in the driver core,
> don't abuse that please...)

I totally agree. This driver is making a whole bunch of assumptions
when it should not make any assumptions. And yes, one of those
assumptions is that the driver of the device has a specific name, and
that is totally crazy. So why is it making those assumptions? I have
no idea, but is does, and they are now causing the first problem -
NULL pointer dereference.

This patch (and that other) is only proposing a simple way to solve
that NULL pointer dereference issue by adding some sanity checks. If
that's no OK, and the whole driver should be refactored instead, then
that is perfectly OK by me, but that has to be done by somebody who
understands what exactly is the driver and the device it's controlling
doing (and for).

thanks,

-- 
heikki