[PATCH 1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt()

[PATCH 1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt()

[Dan Carpenter]

Call release_sock(sk); before returning on this error path.

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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

[PATCH 2/2] Bluetooth: ISO: fix info leak in iso_sock_getsockopt()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
 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

RE: [1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

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

Re: [PATCH 2/2] Bluetooth: ISO: fix info leak in iso_sock_getsockopt()

[Luiz Augusto von Dentz]

Hi Dan,

On Wed, Jul 27, 2022 at 5:10 AM Dan Carpenter <dan.carpenter@oracle.com> 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 <dan.carpenter@oracle.com>
> ---
>  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

Re: [PATCH 1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt()

[patchwork-bot+bluetooth]

Hello:

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

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 <dan.carpenter@oracle.com>
> ---
>  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

Re: [PATCH 2/2] Bluetooth: ISO: fix info leak in iso_sock_getsockopt()

[Dan Carpenter]

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