Call release_sock(sk); before returning on this error path.
Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Dan Carpenter <[email protected]>
---
net/bluetooth/iso.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index ff09c353e64e..19d003727b50 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1177,8 +1177,10 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
}
len = min_t(unsigned int, sizeof(qos), optlen);
- if (len != sizeof(qos))
- return -EINVAL;
+ if (len != sizeof(qos)) {
+ err = -EINVAL;
+ break;
+ }
memset(&qos, 0, sizeof(qos));
--
2.35.1
The "qos" struct has holes after the in and out struct members. Zero
out those holes to prevent leaking stack information.
The C standard rules for when struct holes are zeroed out are slightly
weird. The existing assignments might initialize everything, but GCC
is allowed to (and does sometimes) leave the struct holes uninitialized.
However, when you have a struct initializer that doesn't initialize
every member then the holes must be zeroed.
Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Dan Carpenter <[email protected]>
---
net/bluetooth/iso.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 19d003727b50..c982087d3b52 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1235,7 +1235,7 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
{
struct sock *sk = sock->sk;
int len, err = 0;
- struct bt_iso_qos qos;
+ struct bt_iso_qos qos = {}; /* zero out struct holes */
u8 base_len;
u8 *base;
--
2.35.1
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=663427
---Test result---
Test Summary:
CheckPatch PASS 3.49 seconds
GitLint PASS 1.67 seconds
SubjectPrefix PASS 1.21 seconds
BuildKernel PASS 35.53 seconds
BuildKernel32 PASS 31.02 seconds
Incremental Build with patchesPASS 48.90 seconds
TestRunner: Setup PASS 509.47 seconds
TestRunner: l2cap-tester PASS 17.95 seconds
TestRunner: bnep-tester PASS 6.84 seconds
TestRunner: mgmt-tester PASS 106.23 seconds
TestRunner: rfcomm-tester PASS 10.13 seconds
TestRunner: sco-tester PASS 9.97 seconds
TestRunner: smp-tester PASS 9.88 seconds
TestRunner: userchan-tester PASS 6.87 seconds
---
Regards,
Linux Bluetooth
Hi Dan,
On Wed, Jul 27, 2022 at 5:10 AM Dan Carpenter <[email protected]> wrote:
>
> The "qos" struct has holes after the in and out struct members. Zero
> out those holes to prevent leaking stack information.
>
> The C standard rules for when struct holes are zeroed out are slightly
> weird. The existing assignments might initialize everything, but GCC
> is allowed to (and does sometimes) leave the struct holes uninitialized.
> However, when you have a struct initializer that doesn't initialize
> every member then the holes must be zeroed.
>
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> net/bluetooth/iso.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 19d003727b50..c982087d3b52 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1235,7 +1235,7 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> {
> struct sock *sk = sock->sk;
> int len, err = 0;
> - struct bt_iso_qos qos;
> + struct bt_iso_qos qos = {}; /* zero out struct holes */
> u8 base_len;
> u8 *base;
>
> --
> 2.35.1
Interesting, did you get a report from static analyzer or something?
The variable gets assigned in the code below which has the exact same
size thus I don't see how it would leave anything uninitialized:
if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
qos = iso_pi(sk)->conn->hcon->iso_qos;
else
qos = iso_pi(sk)->qos;
Well perhaps it would have been better to use a pointer though so we
don't have to copy anything:
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index ff09c353e64e..0e4ec46ef273 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1233,7 +1233,7 @@ static int iso_sock_getsockopt(struct socket
*sock, int level, int optname,
{
struct sock *sk = sock->sk;
int len, err = 0;
- struct bt_iso_qos qos;
+ struct bt_iso_qos *qos;
u8 base_len;
u8 *base;
@@ -1259,12 +1259,12 @@ static int iso_sock_getsockopt(struct socket
*sock, int level, int optname,
case BT_ISO_QOS:
if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
- qos = iso_pi(sk)->conn->hcon->iso_qos;
+ qos = &iso_pi(sk)->conn->hcon->iso_qos;
else
- qos = iso_pi(sk)->qos;
+ qos = &iso_pi(sk)->qos;
len = min_t(unsigned int, len, sizeof(qos));
- if (copy_to_user(optval, (char *)&qos, len))
+ if (copy_to_user(optval, (char *)qos, len))
err = -EFAULT;
break;
--
Luiz Augusto von Dentz
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Wed, 27 Jul 2022 15:08:56 +0300 you wrote:
> Call release_sock(sk); before returning on this error path.
>
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> net/bluetooth/iso.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Here is the summary with links:
- [1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt()
https://git.kernel.org/bluetooth/bluetooth-next/c/13474ba176c9
- [2/2] Bluetooth: ISO: fix info leak in iso_sock_getsockopt()
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Wed, Jul 27, 2022 at 12:51:30PM -0700, Luiz Augusto von Dentz wrote:
> Interesting, did you get a report from static analyzer or something?
Yeah. It's a Smatch check. Unfortunately, it still complains after my
patch... Which is frustrating because I thought I had fixed that.
> The variable gets assigned in the code below which has the exact same
> size thus I don't see how it would leave anything uninitialized:
>
> if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
> qos = iso_pi(sk)->conn->hcon->iso_qos;
> else
> qos = iso_pi(sk)->qos;
It's the struct holes after ->in and ->out which are the issue. When
you have an assignment like that, the compiler is allowed to do it as
a series of assignments:
foo = bar;
becomes:
foo.a = bar.a;
foo.b = bar.b;
foo.c = bar.c;
>
> Well perhaps it would have been better to use a pointer though so we
> don't have to copy anything:
That works, and it's faster too. Do you want to send that and give me
a Reported-by tag? Otherwise I can.
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index ff09c353e64e..0e4ec46ef273 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1233,7 +1233,7 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
> {
> struct sock *sk = sock->sk;
> int len, err = 0;
> - struct bt_iso_qos qos;
> + struct bt_iso_qos *qos;
> u8 base_len;
> u8 *base;
>
> @@ -1259,12 +1259,12 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
>
> case BT_ISO_QOS:
> if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
> - qos = iso_pi(sk)->conn->hcon->iso_qos;
> + qos = &iso_pi(sk)->conn->hcon->iso_qos;
> else
> - qos = iso_pi(sk)->qos;
> + qos = &iso_pi(sk)->qos;
>
> len = min_t(unsigned int, len, sizeof(qos));
> - if (copy_to_user(optval, (char *)&qos, len))
> + if (copy_to_user(optval, (char *)qos, len))
No need to cast btw.
regards,
dan carpenter