diff mbox series

Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}()

Message ID 20241002141217.663070-1-andrew.shadura@collabora.co.uk
State New
Headers show
Series Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}() | expand

Commit Message

Andrej Shadura Oct. 2, 2024, 2:12 p.m. UTC 
Commit 9bf4e919ccad worked around an issue introduced after an innocuous
optimisation change in LLVM main:

> 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 same issue occurs in rfcomm in functions rfcomm_sock_bind and
rfcomm_sock_getsockopt_old.

Change the type of len to size_t in both rfcomm_sock_bind and
rfcomm_sock_getsockopt_old and replace min_t() with min().

Cc: stable@vger.kernel.org
Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
Link: https://github.com/ClangBuiltLinux/linux/issues/2007
Link: https://github.com/llvm/llvm-project/issues/85647
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
---
 net/bluetooth/rfcomm/sock.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Nathan Chancellor Oct. 2, 2024, 4:45 p.m. UTC | #1 
On Wed, Oct 02, 2024 at 06:29:22PM +0200, Andrej Shadura wrote:
> > On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
> >> Commit 9bf4e919ccad worked around an issue introduced after an innocuous
> >> optimisation change in LLVM main:
> >>
> >>> 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 same issue occurs in rfcomm in functions rfcomm_sock_bind and
> >> rfcomm_sock_getsockopt_old.
> 
> > Was this found by inspection or is there an actual instance of the same
> > warning? For what it's worth, I haven't seen a warning from this file in
> > any of my build tests.
> 
> We’ve seen build errors in rfcomm in the Chromium OS 4.14 kernel.

If there is any reason to send a v2 (since I don't think my nit below
really warrants one at the moment), you may consider adding this
information and the warning text that you see to the commit message for
future travelers. I definitely have to go back to look at prior commits
for understanding on current issues and I appreciate having easy things
to look for when searching.

Is the warning reproducible on mainline with the same configuration?
Just curious, I still think it is worth sending this through mainline
even if there is no warning currently, since we cannot say that an
potential future LLVM change could not make this show up there if not
properly fixed. I am just wondering what my test coverage is missing :)

> >> Change the type of len to size_t in both rfcomm_sock_bind and
> >> rfcomm_sock_getsockopt_old and replace min_t() with min().
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
> 
> > I am not sure that I totally agree with this Fixes tag since I did not
> > have a warning to fix but I guess it makes sense to help with
> > backporting.
> 
> It’s a shame there isn’t a Complements: or Improves: tag :)

Heh, who is to say we can't be the first? :)

There is some prior art with References: but eh, like I said above, I do
not think it truly matters, since it should make it easier for the
stable folks to apply it. That comment was more directed at how
overloaded Fixes: has become to me that maybe it is worth considering
expanding the type of tags for referring to previous commits but I am
sure there would be push back in some form... No worries regardless!

Cheers,
Nathan
kernel test robot Oct. 4, 2024, 8:41 a.m. UTC | #2 
Hi Andrej,

kernel test robot noticed the following build errors:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on bluetooth/master linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrej-Shadura/Bluetooth-Fix-type-of-len-in-rfcomm_sock_-bind-getsockopt_old/20241002-221656
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
patch link:    https://lore.kernel.org/r/20241002141217.663070-1-andrew.shadura%40collabora.co.uk
patch subject: [PATCH] Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}()
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20241004/202410041637.iOIxEAQQ-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410041637.iOIxEAQQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410041637.iOIxEAQQ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <command-line>:
   net/bluetooth/rfcomm/sock.c: In function 'rfcomm_sock_bind':
>> include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_602' declared with attribute error: min(sizeof(sa), addr_len) signedness error
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                                             ^
   include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert'
     491 |                         prefix ## suffix();                             \
         |                         ^~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:100:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     100 |         BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy),        \
         |         ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:105:9: note: in expansion of macro '__careful_cmp_once'
     105 |         __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:129:25: note: in expansion of macro '__careful_cmp'
     129 | #define min(x, y)       __careful_cmp(min, x, y)
         |                         ^~~~~~~~~~~~~
   net/bluetooth/rfcomm/sock.c:339:15: note: in expansion of macro 'min'
     339 |         len = min(sizeof(sa), addr_len);
         |               ^~~


vim +/__compiletime_assert_602 +510 include/linux/compiler_types.h

eb5c2d4b45e3d2 Will Deacon 2020-07-21  496  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  497  #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21  498  	__compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  499  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  500  /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21  501   * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  502   * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21  503   * @msg:       a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  504   *
eb5c2d4b45e3d2 Will Deacon 2020-07-21  505   * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  506   * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  507   * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21  508   */
eb5c2d4b45e3d2 Will Deacon 2020-07-21  509  #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @510  	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  511
kernel test robot Oct. 4, 2024, 3:13 p.m. UTC | #3 
Hi Andrej,

kernel test robot noticed the following build errors:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on bluetooth/master linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrej-Shadura/Bluetooth-Fix-type-of-len-in-rfcomm_sock_-bind-getsockopt_old/20241002-221656
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
patch link:    https://lore.kernel.org/r/20241002141217.663070-1-andrew.shadura%40collabora.co.uk
patch subject: [PATCH] Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}()
config: arm-randconfig-001-20241004 (https://download.01.org/0day-ci/archive/20241004/202410042221.Phncg973-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project fef3566a25ff0e34fb87339ba5e13eca17cec00f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410042221.Phncg973-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410042221.Phncg973-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/bluetooth/rfcomm/sock.c:32:
   In file included from include/net/bluetooth/bluetooth.h:30:
   In file included from include/net/sock.h:46:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/arm/include/asm/cacheflush.h:10:
   In file included from include/linux/mm.h:2232:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> net/bluetooth/rfcomm/sock.c:339:8: error: call to '__compiletime_assert_549' declared with 'error' attribute: min(sizeof(sa), addr_len) signedness error
     339 |         len = min(sizeof(sa), addr_len);
         |               ^
   include/linux/minmax.h:129:19: note: expanded from macro 'min'
     129 | #define min(x, y)       __careful_cmp(min, x, y)
         |                         ^
   include/linux/minmax.h:105:2: note: expanded from macro '__careful_cmp'
     105 |         __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
         |         ^
   include/linux/minmax.h:100:2: note: expanded from macro '__careful_cmp_once'
     100 |         BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy),        \
         |         ^
   note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:498:2: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |         ^
   include/linux/compiler_types.h:491:4: note: expanded from macro '__compiletime_assert'
     491 |                         prefix ## suffix();                             \
         |                         ^
   <scratch space>:174:1: note: expanded from here
     174 | __compiletime_assert_549
         | ^
   1 warning and 1 error generated.


vim +339 net/bluetooth/rfcomm/sock.c

   326	
   327	static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
   328	{
   329		struct sockaddr_rc sa;
   330		struct sock *sk = sock->sk;
   331		int err = 0;
   332		size_t len;
   333	
   334		if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) ||
   335		    addr->sa_family != AF_BLUETOOTH)
   336			return -EINVAL;
   337	
   338		memset(&sa, 0, sizeof(sa));
 > 339		len = min(sizeof(sa), addr_len);
   340		memcpy(&sa, addr, len);
   341	
   342		BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);
   343	
   344		lock_sock(sk);
   345	
   346		if (sk->sk_state != BT_OPEN) {
   347			err = -EBADFD;
   348			goto done;
   349		}
   350	
   351		if (sk->sk_type != SOCK_STREAM) {
   352			err = -EINVAL;
   353			goto done;
   354		}
   355	
   356		write_lock(&rfcomm_sk_list.lock);
   357	
   358		if (sa.rc_channel &&
   359		    __rfcomm_get_listen_sock_by_addr(sa.rc_channel, &sa.rc_bdaddr)) {
   360			err = -EADDRINUSE;
   361		} else {
   362			/* Save source address */
   363			bacpy(&rfcomm_pi(sk)->src, &sa.rc_bdaddr);
   364			rfcomm_pi(sk)->channel = sa.rc_channel;
   365			sk->sk_state = BT_BOUND;
   366		}
   367	
   368		write_unlock(&rfcomm_sk_list.lock);
   369	
   370	done:
   371		release_sock(sk);
   372		return err;
   373	}
   374
Aleksei Vetrov Oct. 7, 2024, 7:49 p.m. UTC | #4 
Hi Andrej,

On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
> Change the type of len to size_t in both rfcomm_sock_bind and
> rfcomm_sock_getsockopt_old and replace min_t() with min().

rfcomm_sock_bind doesn't use copy_to_user, are you sure it has the same
issue?

> @@ -328,14 +328,15 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
>  {
>  	struct sockaddr_rc sa;
>  	struct sock *sk = sock->sk;
> -	int len, err = 0;
> +	int err = 0;
> +	size_t len;
>  
>  	if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) ||
>  	    addr->sa_family != AF_BLUETOOTH)
>  		return -EINVAL;
>  
>  	memset(&sa, 0, sizeof(sa));
> -	len = min_t(unsigned int, sizeof(sa), addr_len);
> +	len = min(sizeof(sa), addr_len);
>  	memcpy(&sa, addr, len);
>  
>  	BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);

This change produces a compilation error around min expression, as
"kernel test robot" notices below. And I think rfcomm_sock_bind
shouldn't be touched at all, it doesn't use copy_to_user and doesn't
produce compile errors with latest Clang.

> @@ -729,7 +730,8 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u
>  	struct sock *l2cap_sk;
>  	struct l2cap_conn *conn;
>  	struct rfcomm_conninfo cinfo;
> -	int len, err = 0;
> +	int err = 0;
> +	size_t len;
>  	u32 opt;
>  
>  	BT_DBG("sk %p", sk);
> @@ -783,7 +785,7 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u
>  		cinfo.hci_handle = conn->hcon->handle;
>  		memcpy(cinfo.dev_class, 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;
>  

This looks ok. But there is the same pattern in rfcomm_sock_getsockopt
(without old prefix) and it also uses copy_to_user and produces compile
error with latest Clang.

Could you remove rfcomm_sock_bind patch and apply it to
rfcomm_sock_getsockopt instead? Or I can send my version of the patch:
we've encountered the same compile errors in rfcomm_sock_getsockopt and
rfcomm_sock_getsockopt_old after updating Clang and would like to get it
fixed.
Andrej Shadura Oct. 9, 2024, 12:01 p.m. UTC | #5 
On 07/10/2024 21:49, Aleksei Vetrov wrote:
> Hi Andrej,
> 
> On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
>> Change the type of len to size_t in both rfcomm_sock_bind and
>> rfcomm_sock_getsockopt_old and replace min_t() with min().

> rfcomm_sock_bind doesn't use copy_to_user, are you sure it has the same
> issue?

> This change produces a compilation error around min expression, as
> "kernel test robot" notices below. And I think rfcomm_sock_bind
> shouldn't be touched at all, it doesn't use copy_to_user and doesn't
> produce compile errors with latest Clang.

Thanks, you’re right, I went a bit too far here :)
Rebuilding with a different verbosity setting revealed it here as well.

> This looks ok. But there is the same pattern in rfcomm_sock_getsockopt
> (without old prefix) and it also uses copy_to_user and produces compile
> error with latest Clang.
> 
> Could you remove rfcomm_sock_bind patch and apply it to
> rfcomm_sock_getsockopt instead? Or I can send my version of the patch:
> we've encountered the same compile errors in rfcomm_sock_getsockopt and
> rfcomm_sock_getsockopt_old after updating Clang and would like to get it
> fixed.

Interestingly, it didn’t produce an error on my clang version, but sure, 
I am amending the patch and will submit it shortly.
diff mbox series

Patch

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 37d63d768afb..c0fe96673b3c 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -328,14 +328,15 @@  static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
 {
 	struct sockaddr_rc sa;
 	struct sock *sk = sock->sk;
-	int len, err = 0;
+	int err = 0;
+	size_t len;
 
 	if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) ||
 	    addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
 	memset(&sa, 0, sizeof(sa));
-	len = min_t(unsigned int, sizeof(sa), addr_len);
+	len = min(sizeof(sa), addr_len);
 	memcpy(&sa, addr, len);
 
 	BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);
@@ -729,7 +730,8 @@  static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u
 	struct sock *l2cap_sk;
 	struct l2cap_conn *conn;
 	struct rfcomm_conninfo cinfo;
-	int len, err = 0;
+	int err = 0;
+	size_t len;
 	u32 opt;
 
 	BT_DBG("sk %p", sk);
@@ -783,7 +785,7 @@  static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u
 		cinfo.hci_handle = conn->hcon->handle;
 		memcpy(cinfo.dev_class, 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;