| Message ID | 20240214085814.3894917-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dc34ebd5c018b0edf47f39d11083ad8312733034 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] pppoe: Fix memory leak in pppoe_sendmsg() | expand |
On Wed, Feb 14, 2024 at 09:01:50AM +0000, Gavrilov Ilia wrote: > syzbot reports a memory leak in pppoe_sendmsg [1]. > > The problem is in the pppoe_recvmsg() function that handles errors > in the wrong order. For the skb_recv_datagram() function, check > the pointer to skb for NULL first, and then check the 'error' variable, > because the skb_recv_datagram() function can set 'error' > to -EAGAIN in a loop but return a correct pointer to socket buffer > after a number of attempts, though 'error' remains set to -EAGAIN. Reviewed-by: Guillaume Nault <gnault@redhat.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 14 Feb 2024 09:01:50 +0000 you wrote: > syzbot reports a memory leak in pppoe_sendmsg [1]. > > The problem is in the pppoe_recvmsg() function that handles errors > in the wrong order. For the skb_recv_datagram() function, check > the pointer to skb for NULL first, and then check the 'error' variable, > because the skb_recv_datagram() function can set 'error' > to -EAGAIN in a loop but return a correct pointer to socket buffer > after a number of attempts, though 'error' remains set to -EAGAIN. > > [...] Here is the summary with links: - [net] pppoe: Fix memory leak in pppoe_sendmsg() https://git.kernel.org/netdev/net/c/dc34ebd5c018 You are awesome, thank you!
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c index 8e7238e97d0a..2ea4f4890d23 100644 --- a/drivers/net/ppp/pppoe.c +++ b/drivers/net/ppp/pppoe.c @@ -1007,26 +1007,21 @@ static int pppoe_recvmsg(struct socket *sock, struct msghdr *m, struct sk_buff *skb; int error = 0; - if (sk->sk_state & PPPOX_BOUND) { - error = -EIO; - goto end; - } + if (sk->sk_state & PPPOX_BOUND) + return -EIO; skb = skb_recv_datagram(sk, flags, &error); - if (error < 0) - goto end; + if (!skb) + return error; - if (skb) { - total_len = min_t(size_t, total_len, skb->len); - error = skb_copy_datagram_msg(skb, 0, m, total_len); - if (error == 0) { - consume_skb(skb); - return total_len; - } + total_len = min_t(size_t, total_len, skb->len); + error = skb_copy_datagram_msg(skb, 0, m, total_len); + if (error == 0) { + consume_skb(skb); + return total_len; } kfree_skb(skb); -end: return error; }
syzbot reports a memory leak in pppoe_sendmsg [1]. The problem is in the pppoe_recvmsg() function that handles errors in the wrong order. For the skb_recv_datagram() function, check the pointer to skb for NULL first, and then check the 'error' variable, because the skb_recv_datagram() function can set 'error' to -EAGAIN in a loop but return a correct pointer to socket buffer after a number of attempts, though 'error' remains set to -EAGAIN. skb_recv_datagram __skb_recv_datagram // Loop. if (err == -EAGAIN) then // go to the next loop iteration __skb_try_recv_datagram // if (skb != NULL) then return 'skb' // else if a signal is received then // return -EAGAIN Found by InfoTeCS on behalf of Linux Verification Center (linuxtesting.org) with Syzkaller. Link: https://syzkaller.appspot.com/bug?extid=6bdfd184eac7709e5cc9 [1] Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: syzbot+6bdfd184eac7709e5cc9@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=6bdfd184eac7709e5cc9 Signed-off-by: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> --- drivers/net/ppp/pppoe.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)