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

[Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>]

Hi Dan,

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