| Message ID | 20231204065952.GA16224@ubuntu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phonet: Fix Use-After-Free in pep_recvmsg | expand |
Hi, Le 4 décembre 2023 08:59:52 GMT+02:00, Hyunwoo Kim <v4bel@theori.io> a écrit : >Because pep_recvmsg() fetches the skb from pn->ctrlreq_queue >without holding the lock_sock and then frees it, >a race can occur with pep_ioctl(). >A use-after-free for a skb occurs with the following flow. Isn't this the same issue that was reported by Huawei rootlab and for which I already provided a pair of patches to the security list two months ago? TBH, I much prefer the approach in the other patch set, which takes the hit on the ioctl() side rather than the recvmsg()'s. Unfortunately, I have no visibility on what happened or didn't happen after that, since the security list is private. >``` >pep_recvmsg() -> skb_dequeue() -> skb_free_datagram() >pep_ioctl() -> skb_peek() >``` >Fix this by adjusting the scope of lock_sock in pep_recvmsg(). > >Signed-off-by: Hyunwoo Kim <v4bel@theori.io> >--- > net/phonet/pep.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > >diff --git a/net/phonet/pep.c b/net/phonet/pep.c >index faba31f2eff2..212d8a9ddaee 100644 >--- a/net/phonet/pep.c >+++ b/net/phonet/pep.c >@@ -1250,12 +1250,17 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > if (unlikely(1 << sk->sk_state & (TCPF_LISTEN | TCPF_CLOSE))) > return -ENOTCONN; > >+ lock_sock(sk); >+ > if ((flags & MSG_OOB) || sock_flag(sk, SOCK_URGINLINE)) { > /* Dequeue and acknowledge control request */ > struct pep_sock *pn = pep_sk(sk); > >- if (flags & MSG_PEEK) >+ if (flags & MSG_PEEK) { >+ release_sock(sk); > return -EOPNOTSUPP; >+ } >+ Also this change is not really accounted for. > skb = skb_dequeue(&pn->ctrlreq_queue); > if (skb) { > pep_ctrlreq_error(sk, skb, PN_PIPE_NO_ERROR, >@@ -1263,12 +1268,14 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > msg->msg_flags |= MSG_OOB; > goto copy; > } >- if (flags & MSG_OOB) >+ >+ if (flags & MSG_OOB) { >+ release_sock(sk); > return -EINVAL; >+ } > } > > skb = skb_recv_datagram(sk, flags, &err); >- lock_sock(sk); > if (skb == NULL) { > if (err == -ENOTCONN && sk->sk_state == TCP_CLOSE_WAIT) > err = -ECONNRESET; >@@ -1278,7 +1285,7 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > > if (sk->sk_state == TCP_ESTABLISHED) > pipe_grant_credits(sk, GFP_KERNEL); >- release_sock(sk); >+ > copy: > msg->msg_flags |= MSG_EOR; > if (skb->len > len) >@@ -1291,6 +1298,8 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > err = (flags & MSG_TRUNC) ? skb->len : len; > > skb_free_datagram(sk, skb); >+ >+ release_sock(sk); > return err; > } >
Hi, On Mon, Dec 04, 2023 at 09:12:11AM +0200, Rémi Denis-Courmont wrote: > Hi, > > Le 4 décembre 2023 08:59:52 GMT+02:00, Hyunwoo Kim <v4bel@theori.io> a écrit : > >Because pep_recvmsg() fetches the skb from pn->ctrlreq_queue > >without holding the lock_sock and then frees it, > >a race can occur with pep_ioctl(). > >A use-after-free for a skb occurs with the following flow. > > Isn't this the same issue that was reported by Huawei rootlab and for which I already provided a pair of patches to the security list two months ago? Is the issue reported to the security mailing list two months ago the same as this pn->ctrlreq_queue race? > > TBH, I much prefer the approach in the other patch set, which takes the hit on the ioctl() side rather than the recvmsg()'s. That's probably a patch to add sk->sk_receive_queue.lock to pep_ioctl(), is that correct? > > Unfortunately, I have no visibility on what happened or didn't happen after that, since the security list is private. Perhaps this issue hasn't gotten much attention. Regards, Hyunwoo Kim > > >``` > >pep_recvmsg() -> skb_dequeue() -> skb_free_datagram() > >pep_ioctl() -> skb_peek() > >``` > >Fix this by adjusting the scope of lock_sock in pep_recvmsg(). > > > >Signed-off-by: Hyunwoo Kim <v4bel@theori.io> > >--- > > net/phonet/pep.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > >diff --git a/net/phonet/pep.c b/net/phonet/pep.c > >index faba31f2eff2..212d8a9ddaee 100644 > >--- a/net/phonet/pep.c > >+++ b/net/phonet/pep.c > >@@ -1250,12 +1250,17 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > > if (unlikely(1 << sk->sk_state & (TCPF_LISTEN | TCPF_CLOSE))) > > return -ENOTCONN; > > > >+ lock_sock(sk); > >+ > > if ((flags & MSG_OOB) || sock_flag(sk, SOCK_URGINLINE)) { > > /* Dequeue and acknowledge control request */ > > struct pep_sock *pn = pep_sk(sk); > > > >- if (flags & MSG_PEEK) > >+ if (flags & MSG_PEEK) { > >+ release_sock(sk); > > return -EOPNOTSUPP; > >+ } > >+ > > Also this change is not really accounted for. > > > skb = skb_dequeue(&pn->ctrlreq_queue); > > if (skb) { > > pep_ctrlreq_error(sk, skb, PN_PIPE_NO_ERROR, > >@@ -1263,12 +1268,14 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > > msg->msg_flags |= MSG_OOB; > > goto copy; > > } > >- if (flags & MSG_OOB) > >+ > >+ if (flags & MSG_OOB) { > >+ release_sock(sk); > > return -EINVAL; > >+ } > > } > > > > skb = skb_recv_datagram(sk, flags, &err); > >- lock_sock(sk); > > if (skb == NULL) { > > if (err == -ENOTCONN && sk->sk_state == TCP_CLOSE_WAIT) > > err = -ECONNRESET; > >@@ -1278,7 +1285,7 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > > > > if (sk->sk_state == TCP_ESTABLISHED) > > pipe_grant_credits(sk, GFP_KERNEL); > >- release_sock(sk); > >+ > > copy: > > msg->msg_flags |= MSG_EOR; > > if (skb->len > len) > >@@ -1291,6 +1298,8 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > > err = (flags & MSG_TRUNC) ? skb->len : len; > > > > skb_free_datagram(sk, skb); > >+ > >+ release_sock(sk); > > return err; > > } > >
Le keskiviikkona 6. joulukuuta 2023, 6.25.19 EET Hyunwoo Kim a écrit : > Hi, > > On Mon, Dec 04, 2023 at 09:12:11AM +0200, Rémi Denis-Courmont wrote: > > Hi, > > > > Le 4 décembre 2023 08:59:52 GMT+02:00, Hyunwoo Kim <v4bel@theori.io> a écrit : > > >Because pep_recvmsg() fetches the skb from pn->ctrlreq_queue > > >without holding the lock_sock and then frees it, > > >a race can occur with pep_ioctl(). > > >A use-after-free for a skb occurs with the following flow. > > > > Isn't this the same issue that was reported by Huawei rootlab and for > > which I already provided a pair of patches to the security list two > > months ago? > Is the issue reported to the security mailing list two months ago the same > as this pn->ctrlreq_queue race? No, it was another similar problem but the fixes did cover both, I think? > > TBH, I much prefer the approach in the other patch set, which takes the > > hit on the ioctl() side rather than the recvmsg()'s. > That's probably a patch to add sk->sk_receive_queue.lock to pep_ioctl(), is > that correct? More or less > > Unfortunately, I have no visibility on what happened or didn't happen > > after that, since the security list is private. > Perhaps this issue hasn't gotten much attention. Quite possible, but now I'm between a rock and a hard place, because I don't know what's (not) going in the security mailing list. In my understanding, it was not really OK to bring the issue or post the patches on netdev :shrug:
diff --git a/net/phonet/pep.c b/net/phonet/pep.c index faba31f2eff2..212d8a9ddaee 100644 --- a/net/phonet/pep.c +++ b/net/phonet/pep.c @@ -1250,12 +1250,17 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, if (unlikely(1 << sk->sk_state & (TCPF_LISTEN | TCPF_CLOSE))) return -ENOTCONN; + lock_sock(sk); + if ((flags & MSG_OOB) || sock_flag(sk, SOCK_URGINLINE)) { /* Dequeue and acknowledge control request */ struct pep_sock *pn = pep_sk(sk); - if (flags & MSG_PEEK) + if (flags & MSG_PEEK) { + release_sock(sk); return -EOPNOTSUPP; + } + skb = skb_dequeue(&pn->ctrlreq_queue); if (skb) { pep_ctrlreq_error(sk, skb, PN_PIPE_NO_ERROR, @@ -1263,12 +1268,14 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, msg->msg_flags |= MSG_OOB; goto copy; } - if (flags & MSG_OOB) + + if (flags & MSG_OOB) { + release_sock(sk); return -EINVAL; + } } skb = skb_recv_datagram(sk, flags, &err); - lock_sock(sk); if (skb == NULL) { if (err == -ENOTCONN && sk->sk_state == TCP_CLOSE_WAIT) err = -ECONNRESET; @@ -1278,7 +1285,7 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, if (sk->sk_state == TCP_ESTABLISHED) pipe_grant_credits(sk, GFP_KERNEL); - release_sock(sk); + copy: msg->msg_flags |= MSG_EOR; if (skb->len > len) @@ -1291,6 +1298,8 @@ static int pep_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, err = (flags & MSG_TRUNC) ? skb->len : len; skb_free_datagram(sk, skb); + + release_sock(sk); return err; }
Because pep_recvmsg() fetches the skb from pn->ctrlreq_queue without holding the lock_sock and then frees it, a race can occur with pep_ioctl(). A use-after-free for a skb occurs with the following flow. ``` pep_recvmsg() -> skb_dequeue() -> skb_free_datagram() pep_ioctl() -> skb_peek() ``` Fix this by adjusting the scope of lock_sock in pep_recvmsg(). Signed-off-by: Hyunwoo Kim <v4bel@theori.io> --- net/phonet/pep.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)