Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 15:15:47 +0200
Message-ID: <s5ha6d6dzzw.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, Takashi Iwai <tiwai@suse.de>,
        Won Chung <wonchung@google.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Benson Leung <bleung@chromium.org>,
        Prashant Malani <pmalani@chromium.org>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function
In-Reply-To: <YkWj/vmjohNo0r2c@kuha.fi.intel.com>
References: <20220330211913.2068108-1-wonchung@google.com>
        <s5hzgl6eg48.wl-tiwai@suse.de>
        <CAOvb9yiO_n48JPZ3f0+y-fQ_YoOmuWF5c692Jt5_SKbxdA4yAw@mail.gmail.com>
        <s5hr16ieb8o.wl-tiwai@suse.de>
        <YkVzl4NEzwDAp/Zq@kuha.fi.intel.com>
        <YkWTBwAB1HrxcUR3@kroah.com>
        <YkWj/vmjohNo0r2c@kuha.fi.intel.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 31 Mar 2022 14:52:14 +0200,
Heikki Krogerus wrote:
> 
> 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.

Well, it's a sort of best-effort approach for the component framework.
Currently the framework passes a device pointer without knowing what
it is and every master component tries to match with it unless it's
already bound.  Because of that, the driver has to judge which one is
the right one by itself.  The device driver's string is a loose
matching target that practically worked, so far.

Maybe we should define unique subcomponent numbers and rather check
the passed number at matching.  (Though, only relying on the number is
dangerous, too.)


Takashi