linux-kernel - Re: [PATCH v1] usb: typec: altmodes/displayport: verify compatible source/sink role combination

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: <CACeCKac2kknw2s7orH1tu4RsiCr5+WYy1VQ483ok_zuzC2N9zQ@mail.gmail.com>
Date:   Tue, 17 Oct 2023 14:33:18 -0700
From:   Prashant Malani <pmalani@...omium.org>
To:     RD Babiera <rdbabiera@...gle.com>
Cc:     heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org,
        badhri@...gle.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1] usb: typec: altmodes/displayport: verify compatible
 source/sink role combination

Hi RD,

On Mon, Oct 16, 2023 at 3:56 PM RD Babiera <rdbabiera@...gle.com> wrote:
>
> DisplayPort Alt Mode CTS test 10.3.8 states that both sides of the
> connection shall be compatible with one another such that the connection
> is not Source to Source or Sink to Sink.
>
> The DisplayPort driver currently checks for a compatible pin configuration
> that resolves into a source and sink combination. The CTS test is designed
> to send a Discover Modes message that has a compatible pin configuration
> but advertises the same port capability as the device; the current check
> fails this.
>
> Verify that the port and port partner resolve into a valid source and sink
> combination before checking for a compatible pin configuration.
>
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@...r.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@...gle.com>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 718da02036d8..3b35a6b8cb72 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -575,9 +575,18 @@ int dp_altmode_probe(struct typec_altmode *alt)
>         struct fwnode_handle *fwnode;
>         struct dp_altmode *dp;
>         int ret;
> +       int port_cap, partner_cap;

VDOs are 32-bit, so u32 is probably better here.

>
>         /* FIXME: Port can only be DFP_U. */
>
> +       /* Make sure that the port and partner can resolve into source and sink */
> +       port_cap = DP_CAP_CAPABILITY(port->vdo);
> +       partner_cap = DP_CAP_CAPABILITY(alt->vdo);
> +       if (!((port_cap & DP_CAP_DFP_D) && (partner_cap & DP_CAP_UFP_D)) &&

nit: bitwise '&' has a higher precedence than logical '&&', so the
innermost parentheses shouldn't be necessary:

           if (!(port_cap & DP_CAP_DFP_D && partner_cap & DP_CAP_UFP_D) &&
               !(port_cap & DP_CAP_UFP_D && partner_cap & DP_CAP_DFP_D))
                   return -ENODEV;
                ...

OTOH, perhaps you should just introduce a macro that performs this
bitwise operation for even better
readability. Something like

#define DP_CAP_IS_DFP_D(_cap_)        (!!(DP_CAP_CAPABILITY(_cap_) &
DP_CAP_DFP_D))

(not sure if "!!" is tolerated in kernel style, but you get the gist...)


> +           !((port_cap & DP_CAP_UFP_D) && (partner_cap & DP_CAP_DFP_D))) {
> +               return -ENODEV;

Single line if statements can drop curly braces [1]

Best regards,

-Prashant

[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces