[PATCH v3] misc/mei: Add NULL check to component match callback functions

[PATCH v3] misc/mei: Add NULL check to component match callback functions

[Won Chung]

Component match callback functions need to check if expected data is
passed to them. Without this check, it can cause a NULL pointer
dereference when another driver registers a component before i915
drivers have their component master fully bind.

Fixes: 1e8d19d9b0dfc ("mei: hdcp: bind only with i915 on the same PCH")
Fixes: c2004ce99ed73 ("mei: pxp: export pavp client to me client bus")
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Won Chung <wonchung@google.com>
Cc: stable@vger.kernel.org
---
Changes from v2:
- Correctly add "Suggested-by" tag
- Add "Cc: stable@vger.kernel.org"

Changes from v1:
- Add "Fixes" tag
- Send to stable@vger.kernel.org

 drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
 drivers/misc/mei/pxp/mei_pxp.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index ec2a4fce8581..843dbc2b21b1 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
 {
 	struct device *base = data;
 
-	if (strcmp(dev->driver->name, "i915") ||
+	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
 	    subcomponent != I915_COMPONENT_HDCP)
 		return 0;
 
diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
index f7380d387bab..e32a81da8af6 100644
--- a/drivers/misc/mei/pxp/mei_pxp.c
+++ b/drivers/misc/mei/pxp/mei_pxp.c
@@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
 {
 	struct device *base = data;
 
-	if (strcmp(dev->driver->name, "i915") ||
+	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
 	    subcomponent != I915_COMPONENT_PXP)
 		return 0;
 
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Greg KH]

On Thu, Mar 31, 2022 at 08:49:18AM +0000, Won Chung wrote:
> Component match callback functions need to check if expected data is
> passed to them. Without this check, it can cause a NULL pointer
> dereference when another driver registers a component before i915
> drivers have their component master fully bind.

How can that happen in a real system?  Or does this just happen for when
you are doing development and testing?

> 
> Fixes: 1e8d19d9b0dfc ("mei: hdcp: bind only with i915 on the same PCH")
> Fixes: c2004ce99ed73 ("mei: pxp: export pavp client to me client bus")
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Won Chung <wonchung@google.com>
> Cc: stable@vger.kernel.org

Why does this need to go to stable?  How can this be triggered in older
kernels?

> ---
> Changes from v2:
> - Correctly add "Suggested-by" tag
> - Add "Cc: stable@vger.kernel.org"
> 
> Changes from v1:
> - Add "Fixes" tag
> - Send to stable@vger.kernel.org
> 
>  drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
>  drivers/misc/mei/pxp/mei_pxp.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index ec2a4fce8581..843dbc2b21b1 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
>  {
>  	struct device *base = data;
>  
> -	if (strcmp(dev->driver->name, "i915") ||
> +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||

How can base be NULL?


>  	    subcomponent != I915_COMPONENT_HDCP)
>  		return 0;
>  
> diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> index f7380d387bab..e32a81da8af6 100644
> --- a/drivers/misc/mei/pxp/mei_pxp.c
> +++ b/drivers/misc/mei/pxp/mei_pxp.c
> @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
>  {
>  	struct device *base = data;
>  
> -	if (strcmp(dev->driver->name, "i915") ||
> +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||

Same here, shouldn't this be caught by the driver core or bus and match
should not be called?

Why not fix this in the component/driver core instead?

thanks,

greg k-h

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Heikki Krogerus]

Hi Won,

On Thu, Mar 31, 2022 at 08:49:18AM +0000, Won Chung wrote:
> Component match callback functions need to check if expected data is
> passed to them. Without this check, it can cause a NULL pointer
> dereference when another driver registers a component before i915
> drivers have their component master fully bind.
> 
> Fixes: 1e8d19d9b0dfc ("mei: hdcp: bind only with i915 on the same PCH")
> Fixes: c2004ce99ed73 ("mei: pxp: export pavp client to me client bus")
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Won Chung <wonchung@google.com>
> Cc: stable@vger.kernel.org
> ---
> Changes from v2:
> - Correctly add "Suggested-by" tag
> - Add "Cc: stable@vger.kernel.org"
> 
> Changes from v1:
> - Add "Fixes" tag
> - Send to stable@vger.kernel.org

You are sending these really fast. Before you send the next one,
please wait for at least one day.

thanks,

-- 
heikki

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Heikki Krogerus]

On Thu, Mar 31, 2022 at 11:00:46AM +0200, Greg KH wrote:
> On Thu, Mar 31, 2022 at 08:49:18AM +0000, Won Chung wrote:
> > Component match callback functions need to check if expected data is
> > passed to them. Without this check, it can cause a NULL pointer
> > dereference when another driver registers a component before i915
> > drivers have their component master fully bind.
> 
> How can that happen in a real system?  Or does this just happen for when
> you are doing development and testing?
> 
> > 
> > Fixes: 1e8d19d9b0dfc ("mei: hdcp: bind only with i915 on the same PCH")
> > Fixes: c2004ce99ed73 ("mei: pxp: export pavp client to me client bus")
> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Won Chung <wonchung@google.com>
> > Cc: stable@vger.kernel.org
> 
> Why does this need to go to stable?  How can this be triggered in older
> kernels?
> 
> > ---
> > Changes from v2:
> > - Correctly add "Suggested-by" tag
> > - Add "Cc: stable@vger.kernel.org"
> > 
> > Changes from v1:
> > - Add "Fixes" tag
> > - Send to stable@vger.kernel.org
> > 
> >  drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
> >  drivers/misc/mei/pxp/mei_pxp.c   | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> > index ec2a4fce8581..843dbc2b21b1 100644
> > --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> > @@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
> >  {
> >  	struct device *base = data;
> >  
> > -	if (strcmp(dev->driver->name, "i915") ||
> > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> 
> How can base be NULL?
> 
> 
> >  	    subcomponent != I915_COMPONENT_HDCP)
> >  		return 0;
> >  
> > diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> > index f7380d387bab..e32a81da8af6 100644
> > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
> >  {
> >  	struct device *base = data;
> >  
> > -	if (strcmp(dev->driver->name, "i915") ||
> > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> 
> Same here, shouldn't this be caught by the driver core or bus and match
> should not be called?
> 
> Why not fix this in the component/driver core instead?

A component is just a device that is declared to be a "component", and
the code that declares it as component does not have to be the driver
of that device. You simply can't assume that it's bind to a driver
like this function does.

In our case the "components" are USB ports, so devices that are never
bind to drivers.

thanks,

-- 
heikki

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Greg KH]

On Thu, Mar 31, 2022 at 12:32:24PM +0300, Heikki Krogerus wrote:
> On Thu, Mar 31, 2022 at 11:00:46AM +0200, Greg KH wrote:
> > On Thu, Mar 31, 2022 at 08:49:18AM +0000, Won Chung wrote:
> > > Component match callback functions need to check if expected data is
> > > passed to them. Without this check, it can cause a NULL pointer
> > > dereference when another driver registers a component before i915
> > > drivers have their component master fully bind.
> > 
> > How can that happen in a real system?  Or does this just happen for when
> > you are doing development and testing?
> > 
> > > 
> > > Fixes: 1e8d19d9b0dfc ("mei: hdcp: bind only with i915 on the same PCH")
> > > Fixes: c2004ce99ed73 ("mei: pxp: export pavp client to me client bus")
> > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Won Chung <wonchung@google.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Why does this need to go to stable?  How can this be triggered in older
> > kernels?
> > 
> > > ---
> > > Changes from v2:
> > > - Correctly add "Suggested-by" tag
> > > - Add "Cc: stable@vger.kernel.org"
> > > 
> > > Changes from v1:
> > > - Add "Fixes" tag
> > > - Send to stable@vger.kernel.org
> > > 
> > >  drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
> > >  drivers/misc/mei/pxp/mei_pxp.c   | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> > > index ec2a4fce8581..843dbc2b21b1 100644
> > > --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> > > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> > > @@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
> > >  {
> > >  	struct device *base = data;
> > >  
> > > -	if (strcmp(dev->driver->name, "i915") ||
> > > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> > 
> > How can base be NULL?
> > 
> > 
> > >  	    subcomponent != I915_COMPONENT_HDCP)
> > >  		return 0;
> > >  
> > > diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> > > index f7380d387bab..e32a81da8af6 100644
> > > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > > @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
> > >  {
> > >  	struct device *base = data;
> > >  
> > > -	if (strcmp(dev->driver->name, "i915") ||
> > > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> > 
> > Same here, shouldn't this be caught by the driver core or bus and match
> > should not be called?
> > 
> > Why not fix this in the component/driver core instead?
> 
> A component is just a device that is declared to be a "component", and
> the code that declares it as component does not have to be the driver
> of that device. You simply can't assume that it's bind to a driver
> like this function does.
> 
> In our case the "components" are USB ports, so devices that are never
> bind to drivers.

And going off of the driver name is sane?  That feels ripe for bugs and
problems in the future, but hey, I don't understand the need for this
driver to care about another driver at all.

And why is a USB device being passed to something that it thinks is a
PCI device?  That too feels really wrong and ripe for problems.

thanks,

greg k-h

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Hi Greg,

On Thu, Mar 31, 2022 at 01:38:02PM +0200, Greg KH wrote:
> > > >  		return 0;
> > > >  
> > > > diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> > > > index f7380d387bab..e32a81da8af6 100644
> > > > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > > > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > > > @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
> > > >  {
> > > >  	struct device *base = data;
> > > >  
> > > > -	if (strcmp(dev->driver->name, "i915") ||
> > > > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> > > 
> > > Same here, shouldn't this be caught by the driver core or bus and match
> > > should not be called?
> > > 
> > > Why not fix this in the component/driver core instead?
> > 
> > A component is just a device that is declared to be a "component", and
> > the code that declares it as component does not have to be the driver
> > of that device. You simply can't assume that it's bind to a driver
> > like this function does.
> > 
> > In our case the "components" are USB ports, so devices that are never
> > bind to drivers.
> 
> And going off of the driver name is sane?  That feels ripe for bugs and
> problems in the future, but hey, I don't understand the need for this
> driver to care about another driver at all.

I think the component framework is meant to be this loose confederation of
devices, so going into component match, you don't know what the other device
is yet.

The USB drivers and the i915 drivers 100% don't care about each other,
but the framework doesn't know that yet until all the drivers try to match.

> 
> And why is a USB device being passed to something that it thinks is a
> PCI device?  That too feels really wrong and ripe for problems.
> 

The problematic device that's being passed through here is actually the
usb4_port, not a usb device. My guess would be that's why it's getting past any
checks for whether it's a PCI device.

The component framework currently being used by (hdac_i915, mei_hdcp, mei_pxp)
to connect those three devices together, and completely separately, the
component framework is being used by the typec connector class's port mapper.

These two clusters of devices are using the same component framework, but are
not supposed to interact with each other. When we attempted to add the usb4_port
and its retimer in order to link tbt/usb4 to the typec connector, we discovered
this interaction because we happened to build the usb4_port built-in in our
configs, so it does its component_add earlier.

I agree, by the way. This is all a bit ugly.

Thanks,
Benson

> thanks,
> 
> greg k-h

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3] misc/mei: Add NULL check to component match callback functions

[Greg KH]

On Thu, Mar 31, 2022 at 09:04:59AM -0700, Benson Leung wrote:
> Hi Greg,
> 
> On Thu, Mar 31, 2022 at 01:38:02PM +0200, Greg KH wrote:
> > > > >  		return 0;
> > > > >  
> > > > > diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> > > > > index f7380d387bab..e32a81da8af6 100644
> > > > > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > > > > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > > > > @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
> > > > >  {
> > > > >  	struct device *base = data;
> > > > >  
> > > > > -	if (strcmp(dev->driver->name, "i915") ||
> > > > > +	if (!base || !dev->driver || strcmp(dev->driver->name, "i915") ||
> > > > 
> > > > Same here, shouldn't this be caught by the driver core or bus and match
> > > > should not be called?
> > > > 
> > > > Why not fix this in the component/driver core instead?
> > > 
> > > A component is just a device that is declared to be a "component", and
> > > the code that declares it as component does not have to be the driver
> > > of that device. You simply can't assume that it's bind to a driver
> > > like this function does.
> > > 
> > > In our case the "components" are USB ports, so devices that are never
> > > bind to drivers.
> > 
> > And going off of the driver name is sane?  That feels ripe for bugs and
> > problems in the future, but hey, I don't understand the need for this
> > driver to care about another driver at all.
> 
> I think the component framework is meant to be this loose confederation of
> devices, so going into component match, you don't know what the other device
> is yet.
> 
> The USB drivers and the i915 drivers 100% don't care about each other,
> but the framework doesn't know that yet until all the drivers try to match.
> 
> > 
> > And why is a USB device being passed to something that it thinks is a
> > PCI device?  That too feels really wrong and ripe for problems.
> > 
> 
> The problematic device that's being passed through here is actually the
> usb4_port, not a usb device. My guess would be that's why it's getting past any
> checks for whether it's a PCI device.

a usb4 port should not be a pci device.

So fix up the checks, don't do a random "is this the driver name?"
check, look for the real driver pointer or something like that that you
KNOW is the correct match.

> The component framework currently being used by (hdac_i915, mei_hdcp, mei_pxp)
> to connect those three devices together, and completely separately, the
> component framework is being used by the typec connector class's port mapper.
> 
> These two clusters of devices are using the same component framework, but are
> not supposed to interact with each other. When we attempted to add the usb4_port
> and its retimer in order to link tbt/usb4 to the typec connector, we discovered
> this interaction because we happened to build the usb4_port built-in in our
> configs, so it does its component_add earlier.
> 
> I agree, by the way. This is all a bit ugly.

Something is wrong with the component code here if this is happening, as
that's not a solid interface that can actually work at all.

Perhaps do not use this framework until it is fixed?

And again, is this something that you see today on an unmodified 5.17.1
release?  If not, why all of the stable backport requests?

confused,

greg k-h