| Message ID | YuErMEjse5lgAMO3@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] Bluetooth: ISO: unlock on error path in iso_sock_setsockopt() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
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;
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 --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;
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(-)