linux-kernel - Re: [PATCH v2] usb: typec: tcpm: IS_ERR_OR

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] [day] [month] [year] [list]

Date:   Fri, 4 Aug 2023 07:36:29 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Ladislav Michl <oss-lists@...ops.cz>, Jimmy Hu <hhhuuu@...gle.com>
Cc:     heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org,
        kyletso@...gle.com, badhri@...gle.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] usb: typec: tcpm: IS_ERR_OR_NULL check for
 port->partner

On 8/4/23 02:24, Ladislav Michl wrote:
> Hi Jimmy,
> 
> On Fri, Aug 04, 2023 at 08:49:28AM +0000, Jimmy Hu wrote:
>> port->partner may be an error or NULL, so we must check it with
>> IS_ERR_OR_NULL() before dereferencing it.
>>
>> Move the check to the beginning of the tcpm_handle_vdm_request function.
>>
>> Fixes: 5e1d4c49fbc8 ("usb: typec: tcpm: Determine common SVDM Version")
>> Signed-off-by: Jimmy Hu <hhhuuu@...gle.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 829d75ebab42..2c6a0af155ab 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -1683,10 +1683,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>>   			      (VDO_SVDM_VERS(typec_get_negotiated_svdm_version(typec)));
>>   		break;
>>   	case CMDT_RSP_ACK:
>> -		/* silently drop message if we are not connected */
>> -		if (IS_ERR_OR_NULL(port->partner))
>> -			break;
>> -
>>   		tcpm_ams_finish(port);
>>   
>>   		switch (cmd) {
>> @@ -1792,6 +1788,12 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>   	u32 response[8] = { };
>>   	int i, rlen = 0;
>>   
>> +	/* silently drop message if we are not connected */
> 
> ...comment moved.
> 
>> +	if (IS_ERR_OR_NULL(port->partner)) {
>> +		dev_warn(port->dev, "port partner is an error or NULL\n");
> 
> But code is actually not silent. Also, does the verbsity make sense? And
> if it does, is knowing what error port->partner is containing usefull?
> 

Not only that, it also changes behavior radically from ignoring certain messages
while not connected or after partner registration failed to rejecting all VDM
messages, even those not requiring any partner data. It now also ignores all VDM
messages if the state machine is not in READY state. This is a significant functional
change which needs _much_ more explanation than provided in the patch description.

Guenter