* [PATCH net] ppp: ensure minimum packet size in ppp_write() @ 2022-01-05 11:48 Eric Dumazet 2022-01-05 13:19 ` Guillaume Nault 2022-01-06 12:40 ` patchwork-bot+netdevbpf 0 siblings, 2 replies; 7+ messages in thread From: Eric Dumazet @ 2022-01-05 11:48 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski Cc: netdev, Eric Dumazet, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot From: Eric Dumazet <edumazet@google.com> It seems pretty clear ppp layer assumed user space would always be kind to provide enough data in their write() to a ppp device. This patch makes sure user provides at least 2 bytes. It adds PPP_PROTO_LEN macro that could replace in net-next many occurrences of hard-coded 2 value. I replaced only one occurrence to ease backports to stable kernels. The bug manifests in the following report: BUG: KMSAN: uninit-value in ppp_send_frame+0x28d/0x27c0 drivers/net/ppp/ppp_generic.c:1740 ppp_send_frame+0x28d/0x27c0 drivers/net/ppp/ppp_generic.c:1740 __ppp_xmit_process+0x23e/0x4b0 drivers/net/ppp/ppp_generic.c:1640 ppp_xmit_process+0x1fe/0x480 drivers/net/ppp/ppp_generic.c:1661 ppp_write+0x5cb/0x5e0 drivers/net/ppp/ppp_generic.c:513 do_iter_write+0xb0c/0x1500 fs/read_write.c:853 vfs_writev fs/read_write.c:924 [inline] do_writev+0x645/0xe00 fs/read_write.c:967 __do_sys_writev fs/read_write.c:1040 [inline] __se_sys_writev fs/read_write.c:1037 [inline] __x64_sys_writev+0xe5/0x120 fs/read_write.c:1037 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x54/0xd0 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x44/0xae Uninit was created at: slab_post_alloc_hook mm/slab.h:524 [inline] slab_alloc_node mm/slub.c:3251 [inline] __kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974 kmalloc_reserve net/core/skbuff.c:354 [inline] __alloc_skb+0x545/0xf90 net/core/skbuff.c:426 alloc_skb include/linux/skbuff.h:1126 [inline] ppp_write+0x11d/0x5e0 drivers/net/ppp/ppp_generic.c:501 do_iter_write+0xb0c/0x1500 fs/read_write.c:853 vfs_writev fs/read_write.c:924 [inline] do_writev+0x645/0xe00 fs/read_write.c:967 __do_sys_writev fs/read_write.c:1040 [inline] __se_sys_writev fs/read_write.c:1037 [inline] __x64_sys_writev+0xe5/0x120 fs/read_write.c:1037 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x54/0xd0 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x44/0xae Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Paul Mackerras <paulus@samba.org> Cc: linux-ppp@vger.kernel.org Reported-by: syzbot <syzkaller@googlegroups.com> --- drivers/net/ppp/ppp_generic.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 1180a0e2445fbfb3204fea785f1c1cf48bc77141..3ab24988198feaa147397f9ce231815ed1dfa293 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -69,6 +69,8 @@ #define MPHDRLEN 6 /* multilink protocol header length */ #define MPHDRLEN_SSN 4 /* ditto with short sequence numbers */ +#define PPP_PROTO_LEN 2 + /* * An instance of /dev/ppp can be associated with either a ppp * interface unit or a ppp channel. In both cases, file->private_data @@ -497,6 +499,9 @@ static ssize_t ppp_write(struct file *file, const char __user *buf, if (!pf) return -ENXIO; + /* All PPP packets should start with the 2-byte protocol */ + if (count < PPP_PROTO_LEN) + return -EINVAL; ret = -ENOMEM; skb = alloc_skb(count + pf->hdrlen, GFP_KERNEL); if (!skb) @@ -1764,7 +1769,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb) } ++ppp->stats64.tx_packets; - ppp->stats64.tx_bytes += skb->len - 2; + ppp->stats64.tx_bytes += skb->len - PPP_PROTO_LEN; switch (proto) { case PPP_IP: -- 2.34.1.448.ga2b2bfdf31-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 11:48 [PATCH net] ppp: ensure minimum packet size in ppp_write() Eric Dumazet @ 2022-01-05 13:19 ` Guillaume Nault 2022-01-05 15:30 ` James Carlson 2022-01-06 12:40 ` patchwork-bot+netdevbpf 1 sibling, 1 reply; 7+ messages in thread From: Guillaume Nault @ 2022-01-05 13:19 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, Jakub Kicinski, netdev, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot On Wed, Jan 05, 2022 at 03:48:42AM -0800, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > It seems pretty clear ppp layer assumed user space > would always be kind to provide enough data > in their write() to a ppp device. > > This patch makes sure user provides at least > 2 bytes. > > It adds PPP_PROTO_LEN macro that could replace > in net-next many occurrences of hard-coded 2 value. The PPP header can be compressed to only 1 byte, but since 2 bytes is assumed in several parts of the code, rejecting such packets in ppp_xmit() is probably the best we can do. Acked-by: Guillaume Nault <gnault@redhat.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 13:19 ` Guillaume Nault @ 2022-01-05 15:30 ` James Carlson 2022-01-05 16:29 ` Guillaume Nault 0 siblings, 1 reply; 7+ messages in thread From: James Carlson @ 2022-01-05 15:30 UTC (permalink / raw) To: Guillaume Nault, Eric Dumazet Cc: David S . Miller, Jakub Kicinski, netdev, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot On 1/5/22 08:19, Guillaume Nault wrote: > On Wed, Jan 05, 2022 at 03:48:42AM -0800, Eric Dumazet wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> It seems pretty clear ppp layer assumed user space >> would always be kind to provide enough data >> in their write() to a ppp device. >> >> This patch makes sure user provides at least >> 2 bytes. >> >> It adds PPP_PROTO_LEN macro that could replace >> in net-next many occurrences of hard-coded 2 value. > > The PPP header can be compressed to only 1 byte, but since 2 bytes is > assumed in several parts of the code, rejecting such packets in > ppp_xmit() is probably the best we can do. The only ones that can be compressed are those less than 0x0100, which are (intentionally) all network layer protocols. We should be getting only control protocol messages though the user-space interface, not network layer, so I'd say it's not just the best we can do, but indeed the right thing to do by design. -- James Carlson 42.703N 71.076W <carlsonj@workingcode.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 15:30 ` James Carlson @ 2022-01-05 16:29 ` Guillaume Nault 2022-01-05 16:35 ` James Carlson 0 siblings, 1 reply; 7+ messages in thread From: Guillaume Nault @ 2022-01-05 16:29 UTC (permalink / raw) To: James Carlson Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot On Wed, Jan 05, 2022 at 10:30:09AM -0500, James Carlson wrote: > On 1/5/22 08:19, Guillaume Nault wrote: > > On Wed, Jan 05, 2022 at 03:48:42AM -0800, Eric Dumazet wrote: > >> From: Eric Dumazet <edumazet@google.com> > >> > >> It seems pretty clear ppp layer assumed user space > >> would always be kind to provide enough data > >> in their write() to a ppp device. > >> > >> This patch makes sure user provides at least > >> 2 bytes. > >> > >> It adds PPP_PROTO_LEN macro that could replace > >> in net-next many occurrences of hard-coded 2 value. > > > > The PPP header can be compressed to only 1 byte, but since 2 bytes is > > assumed in several parts of the code, rejecting such packets in > > ppp_xmit() is probably the best we can do. > > The only ones that can be compressed are those less than 0x0100, which > are (intentionally) all network layer protocols. We should be getting > only control protocol messages though the user-space interface, not > network layer, so I'd say it's not just the best we can do, but indeed > the right thing to do by design. Well, I know of at least one implementation that used to transmit data by writing on ppp unit file descriptors. That was a hack to work around some other problems. Not a beautiful one, but it worked. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 16:29 ` Guillaume Nault @ 2022-01-05 16:35 ` James Carlson 2022-01-05 17:37 ` Guillaume Nault 0 siblings, 1 reply; 7+ messages in thread From: James Carlson @ 2022-01-05 16:35 UTC (permalink / raw) To: Guillaume Nault Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot On 1/5/22 11:29, Guillaume Nault wrote: > On Wed, Jan 05, 2022 at 10:30:09AM -0500, James Carlson wrote: >> On 1/5/22 08:19, Guillaume Nault wrote: >>> On Wed, Jan 05, 2022 at 03:48:42AM -0800, Eric Dumazet wrote: >>>> From: Eric Dumazet <edumazet@google.com> >>>> >>>> It seems pretty clear ppp layer assumed user space >>>> would always be kind to provide enough data >>>> in their write() to a ppp device. >>>> >>>> This patch makes sure user provides at least >>>> 2 bytes. >>>> >>>> It adds PPP_PROTO_LEN macro that could replace >>>> in net-next many occurrences of hard-coded 2 value. >>> >>> The PPP header can be compressed to only 1 byte, but since 2 bytes is >>> assumed in several parts of the code, rejecting such packets in >>> ppp_xmit() is probably the best we can do. >> >> The only ones that can be compressed are those less than 0x0100, which >> are (intentionally) all network layer protocols. We should be getting >> only control protocol messages though the user-space interface, not >> network layer, so I'd say it's not just the best we can do, but indeed >> the right thing to do by design. > > Well, I know of at least one implementation that used to transmit data > by writing on ppp unit file descriptors. That was a hack to work around > some other problems. Not a beautiful one, but it worked. > So, if you do that sort of hack, then you're constrained to send uncompressed protocol numbers regardless of what's negotiated. That seems like a tiny concession. (And receivers are required to handle uncompressed no matter what LCP negotiation says, per 1661 6.5.) And I'd still maintain that the intended design is that control protocols are handled by the user portion, while network layer protocols are connected in the kernel. -- James Carlson 42.703N 71.076W <carlsonj@workingcode.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 16:35 ` James Carlson @ 2022-01-05 17:37 ` Guillaume Nault 0 siblings, 0 replies; 7+ messages in thread From: Guillaume Nault @ 2022-01-05 17:37 UTC (permalink / raw) To: James Carlson Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev, Eric Dumazet, Paul Mackerras, linux-ppp, syzbot On Wed, Jan 05, 2022 at 11:35:52AM -0500, James Carlson wrote: > On 1/5/22 11:29, Guillaume Nault wrote: > > On Wed, Jan 05, 2022 at 10:30:09AM -0500, James Carlson wrote: > >> On 1/5/22 08:19, Guillaume Nault wrote: > >>> On Wed, Jan 05, 2022 at 03:48:42AM -0800, Eric Dumazet wrote: > >>>> From: Eric Dumazet <edumazet@google.com> > >>>> > >>>> It seems pretty clear ppp layer assumed user space > >>>> would always be kind to provide enough data > >>>> in their write() to a ppp device. > >>>> > >>>> This patch makes sure user provides at least > >>>> 2 bytes. > >>>> > >>>> It adds PPP_PROTO_LEN macro that could replace > >>>> in net-next many occurrences of hard-coded 2 value. > >>> > >>> The PPP header can be compressed to only 1 byte, but since 2 bytes is > >>> assumed in several parts of the code, rejecting such packets in > >>> ppp_xmit() is probably the best we can do. > >> > >> The only ones that can be compressed are those less than 0x0100, which > >> are (intentionally) all network layer protocols. We should be getting > >> only control protocol messages though the user-space interface, not > >> network layer, so I'd say it's not just the best we can do, but indeed > >> the right thing to do by design. > > > > Well, I know of at least one implementation that used to transmit data > > by writing on ppp unit file descriptors. That was a hack to work around > > some other problems. Not a beautiful one, but it worked. > > > > So, if you do that sort of hack, then you're constrained to send > uncompressed protocol numbers regardless of what's negotiated. That > seems like a tiny concession. (And receivers are required to handle > uncompressed no matter what LCP negotiation says, per 1661 6.5.) In the case I was refering to, the program was just retransmitting PPP frames and wasn't supposed to modify the headers. We now have kernel support for that, but it landed only one year ago. Before that, the only option was to write on the ppp fd (btw, that was the channel fd, not the unit, sorry). > And I'd still maintain that the intended design is that control > protocols are handled by the user portion, while network layer protocols > are connected in the kernel. Absolutely, I was just pointing out that the kernel doesn't enforce this design and therefore implementations sometimes ignore it. Anyway, I don't see any problem with refusing to send packets smaller than 2 bytes. Hence my acked-by. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ppp: ensure minimum packet size in ppp_write() 2022-01-05 11:48 [PATCH net] ppp: ensure minimum packet size in ppp_write() Eric Dumazet 2022-01-05 13:19 ` Guillaume Nault @ 2022-01-06 12:40 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 7+ messages in thread From: patchwork-bot+netdevbpf @ 2022-01-06 12:40 UTC (permalink / raw) To: Eric Dumazet; +Cc: davem, kuba, netdev, edumazet, paulus, linux-ppp, syzkaller Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Wed, 5 Jan 2022 03:48:42 -0800 you wrote: > From: Eric Dumazet <edumazet@google.com> > > It seems pretty clear ppp layer assumed user space > would always be kind to provide enough data > in their write() to a ppp device. > > This patch makes sure user provides at least > 2 bytes. > > [...] Here is the summary with links: - [net] ppp: ensure minimum packet size in ppp_write() https://git.kernel.org/netdev/net/c/44073187990d 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] 7+ messages in thread
end of thread, other threads:[~2022-01-06 12:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-05 11:48 [PATCH net] ppp: ensure minimum packet size in ppp_write() Eric Dumazet 2022-01-05 13:19 ` Guillaume Nault 2022-01-05 15:30 ` James Carlson 2022-01-05 16:29 ` Guillaume Nault 2022-01-05 16:35 ` James Carlson 2022-01-05 17:37 ` Guillaume Nault 2022-01-06 12:40 ` 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).