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