| Message ID | 20231113031705.803615-1-willemdebruijn.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c0a2a1b0d631fc460d830f52d06211838874d655 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ppp: limit MRU to 64K | expand |
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;
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!
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;