* [PATCH net] ppp: limit MRU to 64K
@ 2023-11-13 3:16 Willem de Bruijn
2023-11-13 8:09 ` Eric Dumazet
2023-11-13 11:10 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Willem de Bruijn @ 2023-11-13 3:16 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, edumazet, pabeni, linux-ppp, stable, mitch,
mostrows, jchapman, Willem de Bruijn,
syzbot+6177e1f90d92583bcc58
From: Willem de Bruijn <willemb@google.com>
ppp_sync_ioctl allows setting device MRU, but does not sanity check
this input.
Limit to a sane upper bound of 64KB.
No implementation I could find generates larger than 64KB frames.
RFC 2823 mentions an upper bound of PPP over SDL of 64KB based on the
16-bit length field. Other protocols will be smaller, such as PPPoE
(9KB jumbo frame) and PPPoA (18190 maximum CPCS-SDU size, RFC 2364).
PPTP and L2TP encapsulate in IP.
Syzbot managed to trigger alloc warning in __alloc_pages:
if (WARN_ON_ONCE_GFP(order > MAX_ORDER, gfp))
WARNING: CPU: 1 PID: 37 at mm/page_alloc.c:4544 __alloc_pages+0x3ab/0x4a0 mm/page_alloc.c:4544
__alloc_skb+0x12b/0x330 net/core/skbuff.c:651
__netdev_alloc_skb+0x72/0x3f0 net/core/skbuff.c:715
netdev_alloc_skb include/linux/skbuff.h:3225 [inline]
dev_alloc_skb include/linux/skbuff.h:3238 [inline]
ppp_sync_input drivers/net/ppp/ppp_synctty.c:669 [inline]
ppp_sync_receive+0xff/0x680 drivers/net/ppp/ppp_synctty.c:334
tty_ldisc_receive_buf+0x14c/0x180 drivers/tty/tty_buffer.c:390
tty_port_default_receive_buf+0x70/0xb0 drivers/tty/tty_port.c:37
receive_buf drivers/tty/tty_buffer.c:444 [inline]
flush_to_ldisc+0x261/0x780 drivers/tty/tty_buffer.c:494
process_one_work+0x884/0x15c0 kernel/workqueue.c:2630
With call
ioctl$PPPIOCSMRU1(r1, 0x40047452, &(0x7f0000000100)=0x5e6417a8)
Similar code exists in other drivers that implement ppp_channel_ops
ioctl PPPIOCSMRU. Those might also be in scope. Notably excluded from
this are pppol2tp_ioctl and pppoe_ioctl.
This code goes back to the start of git history.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+6177e1f90d92583bcc58@syzkaller.appspotmail.com
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/ppp/ppp_synctty.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
index ea261a628786..52d05ce4a281 100644
--- a/drivers/net/ppp/ppp_synctty.c
+++ b/drivers/net/ppp/ppp_synctty.c
@@ -453,6 +453,10 @@ ppp_sync_ioctl(struct ppp_channel *chan, unsigned int cmd, unsigned long arg)
case PPPIOCSMRU:
if (get_user(val, (int __user *) argp))
break;
+ if (val > U16_MAX) {
+ err = -EINVAL;
+ break;
+ }
if (val < PPP_MRU)
val = PPP_MRU;
ap->mru = val;
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] ppp: limit MRU to 64K
2023-11-13 3:16 [PATCH net] ppp: limit MRU to 64K Willem de Bruijn
@ 2023-11-13 8:09 ` Eric Dumazet
2023-11-13 11:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2023-11-13 8:09 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, kuba, pabeni, linux-ppp, stable, mitch, mostrows,
jchapman, Willem de Bruijn, syzbot+6177e1f90d92583bcc58
On Mon, Nov 13, 2023 at 4:17 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> ppp_sync_ioctl allows setting device MRU, but does not sanity check
> this input.
>
> Limit to a sane upper bound of 64KB.
>
> No implementation I could find generates larger than 64KB frames.
> RFC 2823 mentions an upper bound of PPP over SDL of 64KB based on the
> 16-bit length field. Other protocols will be smaller, such as PPPoE
> (9KB jumbo frame) and PPPoA (18190 maximum CPCS-SDU size, RFC 2364).
> PPTP and L2TP encapsulate in IP.
>
> Syzbot managed to trigger alloc warning in __alloc_pages:
>
> if (WARN_ON_ONCE_GFP(order > MAX_ORDER, gfp))
>
> WARNING: CPU: 1 PID: 37 at mm/page_alloc.c:4544 __alloc_pages+0x3ab/0x4a0 mm/page_alloc.c:4544
>
> __alloc_skb+0x12b/0x330 net/core/skbuff.c:651
> __netdev_alloc_skb+0x72/0x3f0 net/core/skbuff.c:715
> netdev_alloc_skb include/linux/skbuff.h:3225 [inline]
> dev_alloc_skb include/linux/skbuff.h:3238 [inline]
> ppp_sync_input drivers/net/ppp/ppp_synctty.c:669 [inline]
> ppp_sync_receive+0xff/0x680 drivers/net/ppp/ppp_synctty.c:334
> tty_ldisc_receive_buf+0x14c/0x180 drivers/tty/tty_buffer.c:390
> tty_port_default_receive_buf+0x70/0xb0 drivers/tty/tty_port.c:37
> receive_buf drivers/tty/tty_buffer.c:444 [inline]
> flush_to_ldisc+0x261/0x780 drivers/tty/tty_buffer.c:494
> process_one_work+0x884/0x15c0 kernel/workqueue.c:2630
>
> With call
>
> ioctl$PPPIOCSMRU1(r1, 0x40047452, &(0x7f0000000100)=0x5e6417a8)
>
> Similar code exists in other drivers that implement ppp_channel_ops
> ioctl PPPIOCSMRU. Those might also be in scope. Notably excluded from
> this are pppol2tp_ioctl and pppoe_ioctl.
>
> This code goes back to the start of git history.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+6177e1f90d92583bcc58@syzkaller.appspotmail.com
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/ppp/ppp_synctty.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> index ea261a628786..52d05ce4a281 100644
> --- a/drivers/net/ppp/ppp_synctty.c
> +++ b/drivers/net/ppp/ppp_synctty.c
> @@ -453,6 +453,10 @@ ppp_sync_ioctl(struct ppp_channel *chan, unsigned int cmd, unsigned long arg)
> case PPPIOCSMRU:
> if (get_user(val, (int __user *) argp))
> break;
> + if (val > U16_MAX) {
> + err = -EINVAL;
> + break;
> + }
> if (val < PPP_MRU)
> val = PPP_MRU;
> ap->mru = val;
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
Reviewed-by: Eric Dumazet <edumazet@google.com>
It is unclear why ppp_sync_input() does not allocate an skb based on
@count argument though...
diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
index ebcdffdf4f0e0193635d2b479e8a9f7a32703509..79cb7916ff03c9b311109372e3d3e434e3669fa6
100644
--- a/drivers/net/ppp/ppp_synctty.c
+++ b/drivers/net/ppp/ppp_synctty.c
@@ -659,14 +659,14 @@ ppp_sync_input(struct syncppp *ap, const u8
*buf, const u8 *flags, int count)
struct sk_buff *skb;
unsigned char *p;
- if (count == 0)
+ if (count <= 0)
return;
if (ap->flags & SC_LOG_INPKT)
ppp_print_buffer ("receive buffer", buf, count);
/* stuff the chars in the skb */
- skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2);
+ skb = dev_alloc_skb(count + PPP_HDRLEN + 2);
if (!skb) {
printk(KERN_ERR "PPPsync: no memory (input pkt)\n");
goto err;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] ppp: limit MRU to 64K
2023-11-13 3:16 [PATCH net] ppp: limit MRU to 64K Willem de Bruijn
2023-11-13 8:09 ` Eric Dumazet
@ 2023-11-13 11:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-11-13 11:10 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, kuba, edumazet, pabeni, linux-ppp, stable, mitch,
mostrows, jchapman, willemb, syzbot+6177e1f90d92583bcc58
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Sun, 12 Nov 2023 22:16:32 -0500 you wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> ppp_sync_ioctl allows setting device MRU, but does not sanity check
> this input.
>
> Limit to a sane upper bound of 64KB.
>
> [...]
Here is the summary with links:
- [net] ppp: limit MRU to 64K
https://git.kernel.org/netdev/net/c/c0a2a1b0d631
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-13 11:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-13 3:16 [PATCH net] ppp: limit MRU to 64K Willem de Bruijn
2023-11-13 8:09 ` Eric Dumazet
2023-11-13 11:10 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).