[PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[RD Babiera]

When releasing an Alt Mode, typec_altmode_release called by a plug device
will not release the plug Alt Mode, meaning that a port will hold a
reference to a plug Alt Mode even if the port partner is unregistered.
As a result, typec_altmode_get_plug() can return an old plug altmode.

Currently, typec_altmode_put_partner does not raise issues
when unregistering a partner altmode. Looking at the current
implementation:

> static void typec_altmode_put_partner(struct altmode *altmode)
> {
>	struct altmode *partner = altmode->partner;

When called by the partner Alt Mode, then partner evaluates to the port's
Alt Mode. When called by the plug Alt Mode, this also evaluates to the
port's Alt Mode.

>	struct typec_altmode *adev;
>
>	if (!partner)
>		return;
>
>	adev = &partner->adev;

This always evaluates to the port's typec_altmode

>	if (is_typec_plug(adev->dev.parent)) {
>		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
>
>		partner->plug[plug->index] = NULL;

If the routine is called to put the plug's Alt mode and altmode refers to
the plug, then adev referring to the port can never be a typec_plug. If
altmode refers to the port, adev will always refer to the port partner,
which runs the block below.

>	} else {
>		partner->partner = NULL;
>	}
>	put_device(&adev->dev);
> }

When calling typec_altmode_set_partner, a registration always calls
get_device() on the port partner or the plug being registered, therefore
typec_altmode_put_partner should put_device() the same device. By changing
adev to altmode->adev, we make sure to put the correct device and properly
unregister plugs. The reason port partners are always properly
unregistered is because even when adev refers to the port, the port
partner gets nullified in the else block. The port device currently gets
put().

Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
Cc: stable@vger.kernel.org
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
 drivers/usb/typec/class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 2e0451bd336e..803be1943445 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -267,7 +267,7 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	if (!partner)
 		return;
 
-	adev = &partner->adev;
+	adev = &altmode->adev;
 
 	if (is_typec_plug(adev->dev.parent)) {
 		struct typec_plug *plug = to_typec_plug(adev->dev.parent);

base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[Heikki Krogerus]

On Tue, Nov 21, 2023 at 08:39:55PM +0000, RD Babiera wrote:
> When releasing an Alt Mode, typec_altmode_release called by a plug device
> will not release the plug Alt Mode, meaning that a port will hold a
> reference to a plug Alt Mode even if the port partner is unregistered.
> As a result, typec_altmode_get_plug() can return an old plug altmode.
> 
> Currently, typec_altmode_put_partner does not raise issues
> when unregistering a partner altmode. Looking at the current
> implementation:
> 
> > static void typec_altmode_put_partner(struct altmode *altmode)
> > {
> >	struct altmode *partner = altmode->partner;
> 
> When called by the partner Alt Mode, then partner evaluates to the port's
> Alt Mode. When called by the plug Alt Mode, this also evaluates to the
> port's Alt Mode.
> 
> >	struct typec_altmode *adev;
> >
> >	if (!partner)
> >		return;
> >
> >	adev = &partner->adev;
> 
> This always evaluates to the port's typec_altmode
> 
> >	if (is_typec_plug(adev->dev.parent)) {
> >		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
> >
> >		partner->plug[plug->index] = NULL;
> 
> If the routine is called to put the plug's Alt mode and altmode refers to
> the plug, then adev referring to the port can never be a typec_plug. If
> altmode refers to the port, adev will always refer to the port partner,
> which runs the block below.
> 
> >	} else {
> >		partner->partner = NULL;
> >	}
> >	put_device(&adev->dev);
> > }
> 
> When calling typec_altmode_set_partner, a registration always calls
> get_device() on the port partner or the plug being registered, therefore
> typec_altmode_put_partner should put_device() the same device. By changing
> adev to altmode->adev, we make sure to put the correct device and properly
> unregister plugs. The reason port partners are always properly
> unregistered is because even when adev refers to the port, the port
> partner gets nullified in the else block. The port device currently gets
> put().
> 
> Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
>  drivers/usb/typec/class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 2e0451bd336e..803be1943445 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -267,7 +267,7 @@ static void typec_altmode_put_partner(struct altmode *altmode)
>  	if (!partner)
>  		return;
>  
> -	adev = &partner->adev;
> +	adev = &altmode->adev;
>  
>  	if (is_typec_plug(adev->dev.parent)) {
>  		struct typec_plug *plug = to_typec_plug(adev->dev.parent);

Sorry, I may have missed something, but do we need to call this
function with ports at all?

static void typec_altmode_release(struct device *dev)
{
        struct altmode *alt = to_altmode(to_typec_altmode(dev));
 
-       typec_altmode_put_partner(alt);
+       if (!is_typec_port(dev->parent))
+               typec_altmode_put_partner(alt);
 
        altmode_id_remove(alt->adev.dev.parent, alt->id);
        kfree(alt);
        ...

thanks,

-- 
heikki

Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[Christian A. Ehrhardt]

Hi,

I found this mail in the archives after looking at a bug report
that was bisected to the change that resulted from the following
analysis:

https://lore.kernel.org/all/CAP-bSRb3SXpgo_BEdqZB-p1K5625fMegRZ17ZkPE1J8ZYgEHDg@mail.gmail.com/

AFAICS the analysis below is partially flawed

On Tue, Nov 21, 2023 at 08:39:55PM +0000, RD Babiera wrote:
> When releasing an Alt Mode, typec_altmode_release called by a plug device
> will not release the plug Alt Mode, meaning that a port will hold a
> reference to a plug Alt Mode even if the port partner is unregistered.
> As a result, typec_altmode_get_plug() can return an old plug altmode.
> 
> Currently, typec_altmode_put_partner does not raise issues
> when unregistering a partner altmode. Looking at the current
> implementation:
> 
> > static void typec_altmode_put_partner(struct altmode *altmode)
> > {
> >	struct altmode *partner = altmode->partner;
> 
> When called by the partner Alt Mode, then partner evaluates to the port's
> Alt Mode. When called by the plug Alt Mode, this also evaluates to the
> port's Alt Mode.
> 
> >	struct typec_altmode *adev;
> >
> >	if (!partner)
> >		return;
> >
> >	adev = &partner->adev;
> 
> This always evaluates to the port's typec_altmode
> 
> >	if (is_typec_plug(adev->dev.parent)) {
> >		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
> >
> >		partner->plug[plug->index] = NULL;
> 
> If the routine is called to put the plug's Alt mode and altmode refers to
> the plug, then adev referring to the port can never be a typec_plug. If
> altmode refers to the port, adev will always refer to the port partner,
> which runs the block below.
> 
> >	} else {
> >		partner->partner = NULL;
> >	}
> >	put_device(&adev->dev);
> > }

So far everything is fine.

> When calling typec_altmode_set_partner, a registration always calls
> get_device() on the port partner or the plug being registered,

This is wrong. It is the altmode of the plug or partner
that holds a reference to the altmode of the port not the other
way around. The port's altmode has (back) pointers to the altmodes
of its partner and the cable plugs but these are weak references that
do not contribute to the refcount.

> therefore
> typec_altmode_put_partner should put_device() the same device. By changing

Thus this conclusion is wrong. The put_device() used to be correct.

> adev to altmode->adev, we make sure to put the correct device and properly
> unregister plugs. The reason port partners are always properly
> unregistered is because even when adev refers to the port, the port
> partner gets nullified in the else block. The port device currently gets
> put().

Please correct me if I missed something.

       regards    Christian

Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[Heikki Krogerus]

Hi Christian,

On Fri, Dec 29, 2023 at 04:32:16PM +0100, Christian A. Ehrhardt wrote:
> 
> Hi,
> 
> I found this mail in the archives after looking at a bug report
> that was bisected to the change that resulted from the following
> analysis:
> 
> https://lore.kernel.org/all/CAP-bSRb3SXpgo_BEdqZB-p1K5625fMegRZ17ZkPE1J8ZYgEHDg@mail.gmail.com/
> 
> AFAICS the analysis below is partially flawed
> 
> On Tue, Nov 21, 2023 at 08:39:55PM +0000, RD Babiera wrote:
> > When releasing an Alt Mode, typec_altmode_release called by a plug device
> > will not release the plug Alt Mode, meaning that a port will hold a
> > reference to a plug Alt Mode even if the port partner is unregistered.
> > As a result, typec_altmode_get_plug() can return an old plug altmode.
> > 
> > Currently, typec_altmode_put_partner does not raise issues
> > when unregistering a partner altmode. Looking at the current
> > implementation:
> > 
> > > static void typec_altmode_put_partner(struct altmode *altmode)
> > > {
> > >	struct altmode *partner = altmode->partner;
> > 
> > When called by the partner Alt Mode, then partner evaluates to the port's
> > Alt Mode. When called by the plug Alt Mode, this also evaluates to the
> > port's Alt Mode.
> > 
> > >	struct typec_altmode *adev;
> > >
> > >	if (!partner)
> > >		return;
> > >
> > >	adev = &partner->adev;
> > 
> > This always evaluates to the port's typec_altmode
> > 
> > >	if (is_typec_plug(adev->dev.parent)) {
> > >		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
> > >
> > >		partner->plug[plug->index] = NULL;
> > 
> > If the routine is called to put the plug's Alt mode and altmode refers to
> > the plug, then adev referring to the port can never be a typec_plug. If
> > altmode refers to the port, adev will always refer to the port partner,
> > which runs the block below.
> > 
> > >	} else {
> > >		partner->partner = NULL;
> > >	}
> > >	put_device(&adev->dev);
> > > }
> 
> So far everything is fine.
> 
> > When calling typec_altmode_set_partner, a registration always calls
> > get_device() on the port partner or the plug being registered,
> 
> This is wrong. It is the altmode of the plug or partner
> that holds a reference to the altmode of the port not the other
> way around. The port's altmode has (back) pointers to the altmodes
> of its partner and the cable plugs but these are weak references that
> do not contribute to the refcount.
> 
> > therefore
> > typec_altmode_put_partner should put_device() the same device. By changing
> 
> Thus this conclusion is wrong. The put_device() used to be correct.
> 
> > adev to altmode->adev, we make sure to put the correct device and properly
> > unregister plugs. The reason port partners are always properly
> > unregistered is because even when adev refers to the port, the port
> > partner gets nullified in the else block. The port device currently gets
> > put().
> 
> Please correct me if I missed something.

Thanks for checking this. Your analysis sounds correct to me.

RD, I think we need to revert the commmit. If you still see the original
problem, please prepare a new patch.

thanks,

-- 
heikki

Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[RD Babiera]

Hi Heikki,

That sounds good to me. Christian had proposed a fix in another email thread,
so I can post the patch after testing on my end.

Thanks again Christian for the analysis and fix.

Best,
---
RD

[PATCH] usb: typec: Fix double free in typec_altmode_put_partner

[Christian A. Ehrhardt]

The altmode device nodes of a port partner and of cable
plugs hold a reference to the altmode of a port. The port's
altmode contains various back pointers but these do not
contribute to the reference count.

Thus, free the port's altmode device instead of doing a
double free on ourself.

Reported-By: Chris Bainbridge <chris.bainbridge@gmail.com>
Fixes: b17b7fe6dd5c (usb: typec: class: fix typec_altmode_put_partner to put plugs)
Cc: RD Babiera <rdbabiera@google.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/usb/typec/class.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 16a670828dde..2da19feacd91 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -263,11 +263,13 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 {
 	struct altmode *partner = altmode->partner;
 	struct typec_altmode *adev;
+	struct typec_altmode *partner_adev;
 
 	if (!partner)
 		return;
 
 	adev = &altmode->adev;
+	partner_adev = &partner->adev;
 
 	if (is_typec_plug(adev->dev.parent)) {
 		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
@@ -276,7 +278,7 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	} else {
 		partner->partner = NULL;
 	}
-	put_device(&adev->dev);
+	put_device(&partner_adev->dev);
 }
 
 /**
-- 
2.40.1

Re: [PATCH] usb: typec: Fix double free in typec_altmode_put_partner

[RD Babiera]

Hi Christian,

I believe commit 9c6b789e954fae73c548f39332bcc56bdf0d4373 would
need to be reverted to apply this patch, but I'm not sure if that's preferred
over submitting a new version of the problematic patch that combines this
solution instead. Regardless I was able to verify the refcount on my setup.

On Tue, Jan 2, 2024 at 11:30 AM Christian A. Ehrhardt <lk@c--e.de> wrote:
>
> The altmode device nodes of a port partner and of cable
> plugs hold a reference to the altmode of a port. The port's
> altmode contains various back pointers but these do not
> contribute to the reference count.
>
> Thus, free the port's altmode device instead of doing a
> double free on ourself.
>
> Reported-By: Chris Bainbridge <chris.bainbridge@gmail.com>
> Fixes: b17b7fe6dd5c (usb: typec: class: fix typec_altmode_put_partner to put plugs)
> Cc: RD Babiera <rdbabiera@google.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Tested-by: RD Babiera <rdbabiera@google.com>

> ---
>  drivers/usb/typec/class.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 16a670828dde..2da19feacd91 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -263,11 +263,13 @@ static void typec_altmode_put_partner(struct altmode *altmode)
>  {
>         struct altmode *partner = altmode->partner;
>         struct typec_altmode *adev;
> +       struct typec_altmode *partner_adev;
>
>         if (!partner)
>                 return;
>
>         adev = &altmode->adev;
> +       partner_adev = &partner->adev;
>
>         if (is_typec_plug(adev->dev.parent)) {
>                 struct typec_plug *plug = to_typec_plug(adev->dev.parent);
> @@ -276,7 +278,7 @@ static void typec_altmode_put_partner(struct altmode *altmode)
>         } else {
>                 partner->partner = NULL;
>         }
> -       put_device(&adev->dev);
> +       put_device(&partner_adev->dev);
>  }
>
>  /**
> --
> 2.40.1
>

Thanks,
---
rd