diff mbox series

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

Message ID YuEq2Aey0VOrxPB+@kili
State Accepted
Commit c85008a4e748051cddc6be6333f55df476f35362
Headers show
Series [1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt() | expand

Commit Message

Dan Carpenter July 27, 2022, 12:08 p.m. UTC 
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(-)

Comments

bluez.test.bot@gmail.com July 27, 2022, 1:37 p.m. UTC | #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
Luiz Augusto von Dentz July 27, 2022, 7:51 p.m. UTC | #2 
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;
patchwork-bot+bluetooth@kernel.org July 27, 2022, 8 p.m. UTC | #3 
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!
Dan Carpenter July 28, 2022, 6:40 a.m. UTC | #4 
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
diff mbox series

Patch

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));