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>
>
