[v2] Bluetooth: Fix type of len in rfcomm_sock_getsockopt{,_old}()

Commit Message

Andrej Shadura Oct. 9, 2024, 12:14 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_getsockopt and
rfcomm_sock_getsockopt_old.

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

Cc: [e-mail]
Co-authored-by: Aleksei Vetrov <[e-mail]>
Improves: 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 <[e-mail]>
Reviewed-by: Nathan Chancellor <[e-mail]>
---
 net/bluetooth/rfcomm/sock.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

David Laight Oct. 9, 2024, 4:37 p.m. UTC | #1

From: Andrej Shadura
> Sent: 09 October 2024 13:14
> 
> 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.

That can't matter because the value is a small positive integer.

> 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_getsockopt and
> rfcomm_sock_getsockopt_old.
> 
> Change the type of len to size_t in both rfcomm_sock_getsockopt and
> rfcomm_sock_getsockopt_old and replace min_t() with min().

Isn't there still a problem if the application passed a negative length.
You are converting it to a very large unsigned value and then reducing
it to the structure size.
Since the structure size will be less than 2GB it makes no difference
whether the '__user int optlen' is ever converted to 64bits.
I think you are just hiding a bug in a different way.

Note that pretty much all the checks for 'optlen' have treated
negative values as 4 since well before the min() and min_t()
#defines were added.
Look at the tcp code!

I bet that globally fixing the test will cause some important
application that is passing 'on stack garbage' to fail.

	David

> 
> Cc: [e-mail]
> Co-authored-by: Aleksei Vetrov <[e-mail]>
> Improves: 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 <[e-mail]>
> Reviewed-by: Nathan Chancellor <[e-mail]>
> ---
>  net/bluetooth/rfcomm/sock.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 37d63d768afb..5f9d370e09b1 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -729,7 +729,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 +784,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;
> 
> @@ -802,7 +803,8 @@ static int rfcomm_sock_getsockopt(struct socket *sock, int level, int optname, c
>  {
>  	struct sock *sk = sock->sk;
>  	struct bt_security sec;
> -	int len, err = 0;
> +	int err = 0;
> +	size_t len;
> 
>  	BT_DBG("sk %p", sk);
> 
> @@ -827,7 +829,7 @@ static int rfcomm_sock_getsockopt(struct socket *sock, int level, int optname, c
>  		sec.level = rfcomm_pi(sk)->sec_level;
>  		sec.key_size = 0;
> 
> -		len = min_t(unsigned int, len, sizeof(sec));
> +		len = min(len, sizeof(sec));
>  		if (copy_to_user(optval, (char *) &sec, len))
>  			err = -EFAULT;
> 
> --
> 2.43.0
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Andrej Shadura Oct. 10, 2024, 6:51 a.m. UTC | #2

Hello,

On 09/10/2024 18:37, David Laight 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.

> That can't matter because the value is a small positive integer.

I agree that it shouldn’t, but it does in the currently released Clang 
version until the bug is fixed.

Andrej Shadura Oct. 17, 2024, 1:46 p.m. UTC | #3

On 09/10/2024 14:14, 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_getsockopt
> and rfcomm_sock_getsockopt_old.
> 
> Change the type of len to size_t in both rfcomm_sock_getsockopt and
> rfcomm_sock_getsockopt_old and replace min_t() with min().

Any more reviews please? It would be great to have this fix merged :)

Thanks in advance.

Luiz Augusto von Dentz Oct. 17, 2024, 2:39 p.m. UTC | #4

Hi Andrej,

On Thu, Oct 17, 2024 at 9:47 AM Andrej Shadura
<[e-mail]> wrote:
>
> On 09/10/2024 14:14, 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_getsockopt
> > and rfcomm_sock_getsockopt_old.
> >
> > Change the type of len to size_t in both rfcomm_sock_getsockopt and
> > rfcomm_sock_getsockopt_old and replace min_t() with min().
>
> Any more reviews please? It would be great to have this fix merged :)

I was waiting to see if David had any more feedback, but if he doesn't
I'm happy to merge this later today.

> Thanks in advance.
>
> --
> Cheers,
>    Andrej
>

Patch

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 37d63d768afb..5f9d370e09b1 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -729,7 +729,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 +784,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;
 
@@ -802,7 +803,8 @@  static int rfcomm_sock_getsockopt(struct socket *sock, int level, int optname, c
 {
 	struct sock *sk = sock->sk;
 	struct bt_security sec;
-	int len, err = 0;
+	int err = 0;
+	size_t len;
 
 	BT_DBG("sk %p", sk);
 
@@ -827,7 +829,7 @@  static int rfcomm_sock_getsockopt(struct socket *sock, int level, int optname, c
 		sec.level = rfcomm_pi(sk)->sec_level;
 		sec.key_size = 0;
 
-		len = min_t(unsigned int, len, sizeof(sec));
+		len = min(len, sizeof(sec));
 		if (copy_to_user(optval, (char *) &sec, len))
 			err = -EFAULT;