[PATCH net] pppoe: Fix memory leak in pppoe_sendmsg()

[PATCH net] pppoe: Fix memory leak in pppoe_sendmsg()

[Gavrilov Ilia]

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(-)

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;
 }
 
-- 
2.39.2

Re: [PATCH net] pppoe: Fix memory leak in pppoe_sendmsg()

[Guillaume Nault]

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>

Re: [PATCH net] pppoe: Fix memory leak in pppoe_sendmsg()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html