Re: [PATCH bluetooth] Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 1, 2024 at 11:24 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
>
> After an innocuous optimization change in LLVM main (19.0.0), x86_64
> allmodconfig (which enables CONFIG_KCSAN / -fsanitize=thread) fails to
> build due to the checks in check_copy_size():
>
>   In file included from net/bluetooth/sco.c:27:
>   In file included from include/linux/module.h:13:
>   In file included from include/linux/stat.h:19:
>   In file included from include/linux/time.h:60:
>   In file included from include/linux/time32.h:13:
>   In file included from include/linux/timex.h:67:
>   In file included from arch/x86/include/asm/timex.h:6:
>   In file included from arch/x86/include/asm/tsc.h:10:
>   In file included from arch/x86/include/asm/msr.h:15:
>   In file included from include/linux/percpu.h:7:
>   In file included from include/linux/smp.h:118:
>   include/linux/thread_info.h:244:4: error: call to '__bad_copy_from' declared with 'error' attribute: copy source size is too small
>     244 |                         __bad_copy_from();
>         |                         ^
>
> The same exact error occurs in l2cap_sock.c. The copy_to_user()
> statements that are failing come from l2cap_sock_getsockopt_old() and
> sco_sock_getsockopt_old(). This does not occur with GCC with or without
> KCSAN or Clang without KCSAN enabled.
>
> len is defined as an 'int' because it is assigned from
> '__user int *optlen'. However, it is clamped against the result of
> sizeof(), which has a type of 'size_t' ('unsigned long' for 64-bit
> platforms). This is done with min_t() because min() requires compatible
> types, which results in both len and the result of sizeof() being casted
> to 'unsigned int', meaning len changes signs and the result of sizeof()
> is truncated. From there, len is passed to copy_to_user(), which has a
> third parameter type of 'unsigned long', so it is widened and changes
> signs again. This excessive casting in combination with the KCSAN
> instrumentation causes LLVM to fail to eliminate the __bad_copy_from()
> call, failing the build.
>
> The official recommendation from LLVM developers is to consistently use
> long types for all size variables to avoid the unnecessary casting in
> the first place. Change the type of len to size_t in both
> l2cap_sock_getsockopt_old() and sco_sock_getsockopt_old(). This clears
> up the error while allowing min_t() to be replaced with min(), resulting
> in simpler code with no casts and fewer implicit conversions. While len
> is a different type than optlen now, it should result in no functional
> change because the result of sizeof() will clamp all values of optlen in
> the same manner as before.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2007
> Link: https://github.com/llvm/llvm-project/issues/85647
> Signed-off-by: Nathan Chancellor <nathan@xxxxxxxxxx>

Reviewed-by: Justin Stitt <justinstitt@xxxxxxxxxx>

> ---
>  net/bluetooth/l2cap_sock.c | 7 ++++---
>  net/bluetooth/sco.c        | 7 ++++---
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 4287aa6cc988..81193427bf05 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -439,7 +439,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
>         struct l2cap_chan *chan = l2cap_pi(sk)->chan;
>         struct l2cap_options opts;
>         struct l2cap_conninfo cinfo;
> -       int len, err = 0;
> +       int err = 0;
> +       size_t len;
>         u32 opt;
>
>         BT_DBG("sk %p", sk);
> @@ -486,7 +487,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
>
>                 BT_DBG("mode 0x%2.2x", chan->mode);
>
> -               len = min_t(unsigned int, len, sizeof(opts));
> +               len = min(len, sizeof(opts));
>                 if (copy_to_user(optval, (char *) &opts, len))
>                         err = -EFAULT;
>
> @@ -536,7 +537,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname,
>                 cinfo.hci_handle = chan->conn->hcon->handle;
>                 memcpy(cinfo.dev_class, chan->conn->hcon->dev_class, 3);
>
> -               len = min_t(unsigned int, len, sizeof(cinfo));
> +               len = min(len, sizeof(cinfo));
>                 if (copy_to_user(optval, (char *) &cinfo, len))
>                         err = -EFAULT;
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 43daf965a01e..9a72d7f1946c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -967,7 +967,8 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname,
>         struct sock *sk = sock->sk;
>         struct sco_options opts;
>         struct sco_conninfo cinfo;
> -       int len, err = 0;
> +       int err = 0;
> +       size_t len;
>
>         BT_DBG("sk %p", sk);
>
> @@ -989,7 +990,7 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname,
>
>                 BT_DBG("mtu %u", opts.mtu);
>
> -               len = min_t(unsigned int, len, sizeof(opts));
> +               len = min(len, sizeof(opts));
>                 if (copy_to_user(optval, (char *)&opts, len))
>                         err = -EFAULT;
>
> @@ -1007,7 +1008,7 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname,
>                 cinfo.hci_handle = sco_pi(sk)->conn->hcon->handle;
>                 memcpy(cinfo.dev_class, sco_pi(sk)->conn->hcon->dev_class, 3);
>
> -               len = min_t(unsigned int, len, sizeof(cinfo));
> +               len = min(len, sizeof(cinfo));
>                 if (copy_to_user(optval, (char *)&cinfo, len))
>                         err = -EFAULT;
>
>
> ---
> base-commit: 7835fcfd132eb88b87e8eb901f88436f63ab60f7
> change-id: 20240401-bluetooth-fix-len-type-getsockopt_old-73c4a8e60444
>
> Best regards,
> --
> Nathan Chancellor <nathan@xxxxxxxxxx>
>





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux