linux-kernel - Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset

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]

Date:   Fri, 4 Sep 2020 12:41:45 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     ChiYuan Huang <u0084500@...il.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        cy_huang <cy_huang@...htek.com>
Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
 not reset issue

On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> Guenter Roeck <linux@...ck-us.net> 於 2020年9月3日 週四 上午12:57寫道：
>>
>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@...htek.com>
>>>
>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
>>> won't bt reset for some case.
>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
>>> ---
>>> Below's the flow.
>>>
>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED
>>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach()
>>> tcpm_port_is_disconnected() will be called.
>>> But port->attached is still true and port->cc1=open and port->cc2=open
>>>
>>> It cause tcpm_port_is_disconnected return false, then hard_reset_count won't be reset.
>>> After that, tcpm_reset_port() is called.
>>> port->attached become false.
>>>
>>> After that, cc now trigger cc_change event, the hard_reset_count will be kept.
>>> Even tcpm_detach will be called, due to port->attached is false, tcpm_detach()
>>> will directly return.
>>>
>>> CC_EVENT will only trigger drp toggling again.
>>> ---
>>>  drivers/usb/typec/tcpm/tcpm.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index a48e3f90..5c73e1d 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port)
>>>               port->tcpc->set_bist_data(port->tcpc, false);
>>>       }
>>>
>>> -     if (tcpm_port_is_disconnected(port))
>>> -             port->hard_reset_count = 0;
>>> +     port->hard_reset_count = 0;
>>>
>>
>> Doesn't that mean that the state machine will never enter
>> error recovery ?
>>
> I think it does't affect the error recovery.
> All error recovery seems to check pd_capable flag.
> 
>>>From my below case, it's A to C cable only. There is no USBPD contract
> will be estabilished.
> 
> This case occurred following by the below test condition
> Cable -> A to C (default Rp bind to vbus) connected to PC.
> 1. first time plugged in the cable with PC
> It will make HARD_RESET_COUNT  to be equal 2
> 2. And then plug out. At that time HARD_RESET_COUNT is till 2.
> 3. next time plugged in again.
> Due to hard_reset_count is still 2 , after wait_cap_timeout, the state
> eventually changed to SNK_READY.
> But during the state transition, no hard_reset  be sent.
> 
> Defined in the USBPD policy engine, typec transition to USBPD, all
> variables must be reset included hard_reset_count.
> So it expected SNK must send hard_reset again.
> 
> The original code defined hard_reset_count must be reset only when
> tcpm_port_is_disconnected.
> 
> It doesn't make sense that it only occurred in some scenario.
> If tcpm_detach is called, hard_reset count must be reset also.
> 

If a hard reset fails, the state machine may cycle through states
HARD_RESET_SEND, HARD_RESET_START, SRC_HARD_RESET_VBUS_OFF,
SRC_HARD_RESET_VBUS_ON back to SRC_UNATTACHED. In this state,
tcpm_src_detach() and with it tcpm_detach() is called. The hard
reset counter is incremented in HARD_RESET_SEND. If tcpm_detach()
resets the counter, the state machine will keep cycling through hard
resets without ever entering the error recovery state. I am not
entirely sure where the counter should be reset, but tcpm_detach()
seems to be the wrong place.

Guenter

>> Guenter
>>
>>>       tcpm_reset_port(port);
>>>  }
>>> --
>>> 2.7.4
>>>