diff mbox series

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

Message ID 20241009121424.1472485-1-andrew.shadura@collabora.co.uk (mailing list archive)
State Accepted
Commit c440001ad70dbd7723f990101f7fc889e7fcf555
Headers show
Series [v2] Bluetooth: Fix type of len in rfcomm_sock_getsockopt{,_old}() | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Non-standard signature: Co-authored-by: #97: Co-authored-by: Aleksei Vetrov <vvvvvv@google.com> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #98: Improves: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()") WARNING: The commit message has 'stable@', perhaps it also needs a 'Fixes:' tag? total: 0 errors, 3 warnings, 0 checks, 34 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13828279.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint fail WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 26: B1 Line exceeds max length (90>80): "Improves: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")"
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS

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: stable@vger.kernel.org
Co-authored-by: Aleksei Vetrov <vvvvvv@google.com>
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 <andrew.shadura@collabora.co.uk>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
---
 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: stable@vger.kernel.org
> Co-authored-by: Aleksei Vetrov <vvvvvv@google.com>
> 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 <andrew.shadura@collabora.co.uk>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  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
<andrew.shadura@collabora.co.uk> 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
>
patchwork-bot+bluetooth@kernel.org Oct. 21, 2024, 5:10 p.m. UTC | #5 
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed,  9 Oct 2024 14:14:24 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: Fix type of len in rfcomm_sock_getsockopt{,_old}()
    https://git.kernel.org/bluetooth/bluetooth-next/c/c440001ad70d

You are awesome, thank you!
diff mbox series

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;