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
