[PATCH net] ppp: ensure minimum packet size in ppp_write()

[PATCH net] ppp: ensure minimum packet size in ppp_write()

[Eric Dumazet]

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

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[Guillaume Nault]

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>

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[James Carlson]

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>

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[Guillaume Nault]

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.

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[James Carlson]

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>

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[Guillaume Nault]

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.

Re: [PATCH net] ppp: ensure minimum packet size in ppp_write()

[patchwork-bot+netdevbpf]

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