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

[Dan Carpenter <dan.carpenter@oracle.com>]

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