linux-kernel - Re: [PATCH 00/15] usb: serial: avoid using usb_control

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 Dec 2020 10:09:29 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Himadri Pandya <himadrispandya@...il.com>
Cc:     johan@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

Hi Himadri,

and sorry about the late feedback on this one. I'm still trying to dig
myself out of some backlog.

On Wed, Nov 04, 2020 at 12:16:48PM +0530, Himadri Pandya wrote:
> There are many usages of usb_control_msg() that can use the new wrapper
> functions usb_contro_msg_send() & usb_control_msg_recv() for better
> error checks on short reads and writes. Hence use them whenever possible
> and avoid using usb_control_msg() directly.

Replacing working code with shiny new helpers unfortunately often ends
up introducing new bugs and I'm afraid there are a few examples of that
in this series as well.

I'll comment on the patches individually, but my impression is that we
should primarily use these helpers to replace code which allocates a
temporary buffer for each transfer as otherwise there's no clear gain
from using them.

Some of your patches contains unrelated changes which needs to go in
separate patches if they're to be included at all.

Please also try to write dedicated commit messages rater than reusing
more or less the same stock message since not everything in these
messages apply to each patch. You never mention that these helpers
nicely hides the allocation of temporary transfer buffers in some cases
for examples. In other places they instead introduce additional
allocations which at least should have been highlighted.

> Himadri Pandya (15):
>   usb: serial: ark3116: use usb_control_msg_recv() and
>     usb_control_msg_send()

Nit: please also use an uppercase "USB" prefix.

>   usb: serial: belkin_sa: use usb_control_msg_send()
>   usb: serial: ch314: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: cp210x: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: cypress_m8: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: f81232: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: f81534: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ftdi_sio: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_edgeport: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_ti: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ipaq: use usb_control_msg_send()
>   usb: serial: ipw: use usb_control_msg_send()
>   usb: serial: iuu_phoenix: use usb_control_msg_send()
>   usb: serial: keyspan_pda: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: kl5kusb105: use usb_control_msg_recv() and
>     usb_control_msg_send()
> 
>  drivers/usb/serial/ark3116.c     |  29 +----
>  drivers/usb/serial/belkin_sa.c   |  35 +++---
>  drivers/usb/serial/ch341.c       |  45 +++-----
>  drivers/usb/serial/cp210x.c      | 148 +++++++------------------
>  drivers/usb/serial/cypress_m8.c  |  38 ++++---
>  drivers/usb/serial/f81232.c      |  88 +++------------
>  drivers/usb/serial/f81534.c      |  63 +++--------
>  drivers/usb/serial/ftdi_sio.c    | 182 +++++++++++++------------------
>  drivers/usb/serial/io_edgeport.c |  73 +++++--------
>  drivers/usb/serial/io_ti.c       |  28 ++---
>  drivers/usb/serial/ipaq.c        |   9 +-
>  drivers/usb/serial/ipw.c         | 107 ++++++------------
>  drivers/usb/serial/iuu_phoenix.c |   5 +-
>  drivers/usb/serial/keyspan_pda.c | 172 ++++++++++++-----------------
>  drivers/usb/serial/kl5kusb105.c  |  94 ++++++++--------
>  15 files changed, 406 insertions(+), 710 deletions(-)

Johan