Re: [PATCH] usb: typec: Prevent setting invalid opmode value

[Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>]

On Thu, Oct 22, 2020 at 03:12:14PM +0530, Sriharsha Allenki wrote:
> Setting opmode to invalid values would lead to a
> paging fault failure when there is an access to the
> power_operation_mode.
> 
> Prevent this by checking the validity of the value
> that the opmode is being set.
> 
> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: fab9288428ec ("usb: USB Type-C connector class")
> Signed-off-by: Sriharsha Allenki <sallenki@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/typec/class.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec70..63efe16 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1427,7 +1427,8 @@ void typec_set_pwr_opmode(struct typec_port *port,
>  {
>  	struct device *partner_dev;
>  
> -	if (port->pwr_opmode == opmode)
> +	if ((port->pwr_opmode == opmode) || (opmode < TYPEC_PWR_MODE_USB) ||

You don't need to check if opmode < anything. opmode is enum which
apparently means that GCC handles it as unsigned. Since
TYPEC_PWR_MODE_USB is 0 it means opmode < TYPEC_PWR_MODE_USB is never
true.

> +						(opmode > TYPEC_PWR_MODE_PD))
>  		return;

You really need to print an error at the very least. Otherwise we will
just silently hide possible driver bugs.

To be honest, I'm not a big fan of this kind of checks. They have
created more problems than they have fixed in more than one occasion
to me. For example, there really is no guarantee that the maximum will
always be TYPEC_PWR_MODE_PD, which means we probable should have
something like TYPEC_PWR_MODE_MAX defined somewhere that you compare
the opmode value to instead of TYPEC_PWR_MODE_PD to play it safe, but
let's not bother with that for now (it will create other problems).

Basically, with functions like this, especially since it doesn't
return anything, the responsibility of checking the validity of the
parameters that the caller supplies to it belongs to the caller IMO,
not the function itself. I would be happy to explain that in the
kernel doc style comment of the function.

If you still feel that this change is really necessary, meaning you
have some actual case where the caller can _not_ check the range
before calling this function, then explain the case you have carefully
in the commit message and add the check as a separate condition:

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec707cb512..7de6913d90f9c 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1430,6 +1430,11 @@ void typec_set_pwr_opmode(struct typec_port *port,
        if (port->pwr_opmode == opmode)
                return;
 
+       if (opmode > TYPEC_PWR_OPMODE_PD) {
+               dev_err(&port->dev, "blah-blah-blah\n");
+               return;
+       }
+
        port->pwr_opmode = opmode;
        sysfs_notify(&port->dev.kobj, NULL, "power_operation_mode");
        kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);

Otherwise you can just propose a patch that improves the documentation
of this function, explaining that it does not take any responsibility
over the parameters passed to it for now.


thanks,

-- 
heikki