linux-kernel - Re: [PATCH] usb: typec: ucsi: Clear pending after acking connector change

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <eebb4180ca4aa6d9f9dc488d7d91199de5731b5c.camel@redhat.com>
Date:   Mon, 17 May 2021 14:36:50 +0200
From:   Benjamin Berg <bberg@...hat.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: typec: ucsi: Clear pending after acking connector
 change

On Mon, 2021-05-17 at 13:03 +0300, Heikki Krogerus wrote:
> On Sat, May 15, 2021 at 09:09:53PM -0700, Bjorn Andersson wrote:
> > It's possible that the interrupt handler for the UCSI driver
> > signals a
> > connector changes after the handler clears the PENDING bit, but
> > before
> > it has sent the acknowledge request. The result is that the handler
> > is
> > invoked yet again, to ack the same connector change.
> > 
> > At least some versions of the Qualcomm UCSI firmware will not
> > handle the
> > second - "spurious" - acknowledgment gracefully. So make sure to
> > not
> > clear the pending flag untuntil the changeil the change is
> > acknowledged.
> > 
> > Any connector changes coming in after the acknowledgment, that
> > would
> > have the pending flag incorrectly cleared, would afaict be covered
> > by
> > the subsequent connector status check.
> > 
> > Fixes: 217504a05532 ("usb: typec: ucsi: Work around PPM losing
> > change information")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> 
> I'm OK with this if Bejamin does not see any problems with it. I'll
> wait for his comments before giving my reviewed-by tag.

Interesting problem. I wonder if we can detect this situation and
restart the worker instead of sending the ACK.

As for the patch, I do believe it is safe. I agree with the assessment
in the commit message, that the subsequent connector check will detect
any race condition and recovers from it.

Acked-By: Benjamin Berg <bberg@...hat.com>

Benjamin

> That workaround (commit 217504a05532) is unfortunately too fragile.
> I'm going to now separate the processing of the connector state from
> the event handler (interrupt handler). That way we should be fairly
> sure we don't loose any of the connector states even if an event is
> generated while we are still in the middle of processing the previous
> one(s), and at the same time be sure that we also don't confuse the
> firmware.
> So the event handler shall after that only read the connector status,
> schedule the unique job where it's processed and ACK the event.
> Nothing else.
> 
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c
> > b/drivers/usb/typec/ucsi/ucsi.c
> > index 282c3c825c13..f451ce0132a9 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -694,8 +694,8 @@ static void ucsi_handle_connector_change(struct
> > work_struct *work)
> >         ucsi_send_command(con->ucsi, command, NULL, 0);
> >  
> >         /* 3. ACK connector change */
> > -       clear_bit(EVENT_PENDING, &ucsi->flags);
> >         ret = ucsi_acknowledge_connector_change(ucsi);
> > +       clear_bit(EVENT_PENDING, &ucsi->flags);
> >         if (ret) {
> >                 dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__,
> > ret);
> >                 goto out_unlock;
> > -- 
> > 2.29.2
> 
> thanks,
> 


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)