* [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter()
@ 2021-02-02 9:20 Sabyrzhan Tasbolatov
2021-02-04 0:28 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Sabyrzhan Tasbolatov @ 2021-02-02 9:20 UTC (permalink / raw)
To: davem, kuba; +Cc: netdev, linux-kernel, syzbot+c2a7e5c5211605a90865
syzbot found WARNING in qrtr_tun_write_iter [1] when write_iter length
exceeds KMALLOC_MAX_SIZE causing order >= MAX_ORDER condition.
Additionally, there is no check for 0 length write.
[1]
WARNING: mm/page_alloc.c:5011
[..]
Call Trace:
alloc_pages_current+0x18c/0x2a0 mm/mempolicy.c:2267
alloc_pages include/linux/gfp.h:547 [inline]
kmalloc_order+0x2e/0xb0 mm/slab_common.c:837
kmalloc_order_trace+0x14/0x120 mm/slab_common.c:853
kmalloc include/linux/slab.h:557 [inline]
kzalloc include/linux/slab.h:682 [inline]
qrtr_tun_write_iter+0x8a/0x180 net/qrtr/tun.c:83
call_write_iter include/linux/fs.h:1901 [inline]
Reported-by: syzbot+c2a7e5c5211605a90865@syzkaller.appspotmail.com
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
net/qrtr/tun.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
index 15ce9b642b25..b238c40a9984 100644
--- a/net/qrtr/tun.c
+++ b/net/qrtr/tun.c
@@ -80,6 +80,12 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
ssize_t ret;
void *kbuf;
+ if (!len)
+ return -EINVAL;
+
+ if (len > KMALLOC_MAX_SIZE)
+ return -ENOMEM;
+
kbuf = kzalloc(len, GFP_KERNEL);
if (!kbuf)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter()
2021-02-02 9:20 [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter() Sabyrzhan Tasbolatov
@ 2021-02-04 0:28 ` Jakub Kicinski
2021-02-04 9:02 ` [PATCH] net/qrtr: replaced useless kzalloc with kmalloc " Sabyrzhan Tasbolatov
2021-02-04 0:40 ` [PATCH] net/qrtr: restrict user-controlled length " patchwork-bot+netdevbpf
2021-02-12 11:51 ` Eric Dumazet
2 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-02-04 0:28 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov
Cc: davem, netdev, linux-kernel, syzbot+c2a7e5c5211605a90865
On Tue, 2 Feb 2021 15:20:59 +0600 Sabyrzhan Tasbolatov wrote:
> syzbot found WARNING in qrtr_tun_write_iter [1] when write_iter length
> exceeds KMALLOC_MAX_SIZE causing order >= MAX_ORDER condition.
>
> Additionally, there is no check for 0 length write.
>
> [1]
> WARNING: mm/page_alloc.c:5011
> [..]
> Call Trace:
> alloc_pages_current+0x18c/0x2a0 mm/mempolicy.c:2267
> alloc_pages include/linux/gfp.h:547 [inline]
> kmalloc_order+0x2e/0xb0 mm/slab_common.c:837
> kmalloc_order_trace+0x14/0x120 mm/slab_common.c:853
> kmalloc include/linux/slab.h:557 [inline]
> kzalloc include/linux/slab.h:682 [inline]
> qrtr_tun_write_iter+0x8a/0x180 net/qrtr/tun.c:83
> call_write_iter include/linux/fs.h:1901 [inline]
>
> Reported-by: syzbot+c2a7e5c5211605a90865@syzkaller.appspotmail.com
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> net/qrtr/tun.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> index 15ce9b642b25..b238c40a9984 100644
> --- a/net/qrtr/tun.c
> +++ b/net/qrtr/tun.c
> @@ -80,6 +80,12 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
> ssize_t ret;
> void *kbuf;
>
> + if (!len)
> + return -EINVAL;
> +
> + if (len > KMALLOC_MAX_SIZE)
> + return -ENOMEM;
> +
> kbuf = kzalloc(len, GFP_KERNEL);
For potential, separate clean up - this is followed
by copy_from_iter_full(len) kzalloc() can probably
be replaced by kmalloc()?
> if (!kbuf)
> return -ENOMEM;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter()
2021-02-02 9:20 [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter() Sabyrzhan Tasbolatov
2021-02-04 0:28 ` Jakub Kicinski
@ 2021-02-04 0:40 ` patchwork-bot+netdevbpf
2021-02-12 11:51 ` Eric Dumazet
2 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-02-04 0:40 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov
Cc: davem, kuba, netdev, linux-kernel, syzbot+c2a7e5c5211605a90865
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Tue, 2 Feb 2021 15:20:59 +0600 you wrote:
> syzbot found WARNING in qrtr_tun_write_iter [1] when write_iter length
> exceeds KMALLOC_MAX_SIZE causing order >= MAX_ORDER condition.
>
> Additionally, there is no check for 0 length write.
>
> [1]
> WARNING: mm/page_alloc.c:5011
> [..]
> Call Trace:
> alloc_pages_current+0x18c/0x2a0 mm/mempolicy.c:2267
> alloc_pages include/linux/gfp.h:547 [inline]
> kmalloc_order+0x2e/0xb0 mm/slab_common.c:837
> kmalloc_order_trace+0x14/0x120 mm/slab_common.c:853
> kmalloc include/linux/slab.h:557 [inline]
> kzalloc include/linux/slab.h:682 [inline]
> qrtr_tun_write_iter+0x8a/0x180 net/qrtr/tun.c:83
> call_write_iter include/linux/fs.h:1901 [inline]
>
> [...]
Here is the summary with links:
- net/qrtr: restrict user-controlled length in qrtr_tun_write_iter()
https://git.kernel.org/netdev/net/c/2a80c1581237
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] 9+ messages in thread
* [PATCH] net/qrtr: replaced useless kzalloc with kmalloc in qrtr_tun_write_iter()
2021-02-04 0:28 ` Jakub Kicinski
@ 2021-02-04 9:02 ` Sabyrzhan Tasbolatov
2021-02-04 18:51 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Sabyrzhan Tasbolatov @ 2021-02-04 9:02 UTC (permalink / raw)
To: kuba; +Cc: davem, linux-kernel, netdev, snovitoll, syzbot+c2a7e5c5211605a90865
Replaced kzalloc() with kmalloc(), there is no need for zeroed-out
memory for simple void *kbuf.
>For potential, separate clean up - this is followed
>by copy_from_iter_full(len) kzalloc() can probably
>be replaced by kmalloc()?
>
>> if (!kbuf)
>> return -ENOMEM;
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
net/qrtr/tun.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
index b238c40a9984..9b607c7614de 100644
--- a/net/qrtr/tun.c
+++ b/net/qrtr/tun.c
@@ -86,7 +86,7 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (len > KMALLOC_MAX_SIZE)
return -ENOMEM;
- kbuf = kzalloc(len, GFP_KERNEL);
+ kbuf = kmalloc(len, GFP_KERNEL);
if (!kbuf)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: replaced useless kzalloc with kmalloc in qrtr_tun_write_iter()
2021-02-04 9:02 ` [PATCH] net/qrtr: replaced useless kzalloc with kmalloc " Sabyrzhan Tasbolatov
@ 2021-02-04 18:51 ` Jakub Kicinski
2021-02-04 18:53 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-02-04 18:51 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov
Cc: davem, linux-kernel, netdev, syzbot+c2a7e5c5211605a90865
On Thu, 4 Feb 2021 15:02:30 +0600 Sabyrzhan Tasbolatov wrote:
> Replaced kzalloc() with kmalloc(), there is no need for zeroed-out
> memory for simple void *kbuf.
There is no need for zeroed-out memory because it's immediately
overwritten by copy_from_iter...
> >For potential, separate clean up - this is followed
> >by copy_from_iter_full(len) kzalloc() can probably
> >be replaced by kmalloc()?
> >
> >> if (!kbuf)
> >> return -ENOMEM;
>
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> net/qrtr/tun.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> index b238c40a9984..9b607c7614de 100644
> --- a/net/qrtr/tun.c
> +++ b/net/qrtr/tun.c
> @@ -86,7 +86,7 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
> if (len > KMALLOC_MAX_SIZE)
> return -ENOMEM;
>
> - kbuf = kzalloc(len, GFP_KERNEL);
> + kbuf = kmalloc(len, GFP_KERNEL);
> if (!kbuf)
> return -ENOMEM;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: replaced useless kzalloc with kmalloc in qrtr_tun_write_iter()
2021-02-04 18:51 ` Jakub Kicinski
@ 2021-02-04 18:53 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2021-02-04 18:53 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov
Cc: davem, linux-kernel, netdev, syzbot+c2a7e5c5211605a90865
On Thu, 4 Feb 2021 10:51:59 -0800 Jakub Kicinski wrote:
> On Thu, 4 Feb 2021 15:02:30 +0600 Sabyrzhan Tasbolatov wrote:
> > Replaced kzalloc() with kmalloc(), there is no need for zeroed-out
> > memory for simple void *kbuf.
>
> There is no need for zeroed-out memory because it's immediately
> overwritten by copy_from_iter...
Also if you don't mind please wait a week until the fixes get merged
back into net-next and then repost. Otherwise this patch will not apply
cleanly. (Fixes are merged into a different tree than cleanups)
> > Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> > ---
> > net/qrtr/tun.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> > index b238c40a9984..9b607c7614de 100644
> > --- a/net/qrtr/tun.c
> > +++ b/net/qrtr/tun.c
> > @@ -86,7 +86,7 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > if (len > KMALLOC_MAX_SIZE)
> > return -ENOMEM;
> >
> > - kbuf = kzalloc(len, GFP_KERNEL);
> > + kbuf = kmalloc(len, GFP_KERNEL);
> > if (!kbuf)
> > return -ENOMEM;
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter()
2021-02-02 9:20 [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter() Sabyrzhan Tasbolatov
2021-02-04 0:28 ` Jakub Kicinski
2021-02-04 0:40 ` [PATCH] net/qrtr: restrict user-controlled length " patchwork-bot+netdevbpf
@ 2021-02-12 11:51 ` Eric Dumazet
2021-02-21 12:39 ` [PATCH] net/qrtr: restrict " Sabyrzhan Tasbolatov
2 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2021-02-12 11:51 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov, davem, kuba
Cc: netdev, linux-kernel, syzbot+c2a7e5c5211605a90865
On 2/2/21 10:20 AM, Sabyrzhan Tasbolatov wrote:
> syzbot found WARNING in qrtr_tun_write_iter [1] when write_iter length
> exceeds KMALLOC_MAX_SIZE causing order >= MAX_ORDER condition.
>
> Additionally, there is no check for 0 length write.
>
> [1]
> WARNING: mm/page_alloc.c:5011
> [..]
> Call Trace:
> alloc_pages_current+0x18c/0x2a0 mm/mempolicy.c:2267
> alloc_pages include/linux/gfp.h:547 [inline]
> kmalloc_order+0x2e/0xb0 mm/slab_common.c:837
> kmalloc_order_trace+0x14/0x120 mm/slab_common.c:853
> kmalloc include/linux/slab.h:557 [inline]
> kzalloc include/linux/slab.h:682 [inline]
> qrtr_tun_write_iter+0x8a/0x180 net/qrtr/tun.c:83
> call_write_iter include/linux/fs.h:1901 [inline]
>
> Reported-by: syzbot+c2a7e5c5211605a90865@syzkaller.appspotmail.com
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> net/qrtr/tun.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
> index 15ce9b642b25..b238c40a9984 100644
> --- a/net/qrtr/tun.c
> +++ b/net/qrtr/tun.c
> @@ -80,6 +80,12 @@ static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
> ssize_t ret;
> void *kbuf;
>
> + if (!len)
> + return -EINVAL;
> +
> + if (len > KMALLOC_MAX_SIZE)
> + return -ENOMEM;
Probably not enough.
qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
some extra space (for struct skb_shared_info)
Do we really expect to accept huge lengths here ?
> +
> kbuf = kzalloc(len, GFP_KERNEL);
> if (!kbuf)
> return -ENOMEM;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: restrict length in qrtr_tun_write_iter()
2021-02-12 11:51 ` Eric Dumazet
@ 2021-02-21 12:39 ` Sabyrzhan Tasbolatov
2021-02-22 8:45 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Sabyrzhan Tasbolatov @ 2021-02-21 12:39 UTC (permalink / raw)
To: eric.dumazet
Cc: davem, kuba, linux-kernel, netdev, snovitoll,
syzbot+c2a7e5c5211605a90865
> Do we really expect to accept huge lengths here ?
Sorry for late response but I couldnt find any reference to the max
length of incoming data for qrtr TUN interface.
> qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
> some extra space (for struct skb_shared_info)
Thanks, you're right, qrtr_endpoint_post() will alloc another slab buffer.
We can check the length of skb allocation but we need to do following:
int qrtr_endpoint_post(.., const void *data, size_t len)
{
..
when QRTR_PROTO_VER_1:
hdrlen = sizeof(*data);
when QRTR_PROTO_VER_2:
hdrlen = sizeof(*data) + data->optlen;
len = (KMALLOC_MAX_SIZE - hdrlen) % data->size;
skb = netdev_alloc_skb(NULL, len);
..
skb_put_data(skb, data + hdrlen, size);
So it requires refactoring as in qrtr_tun_write_iter() we just allocate and
pass it to qrtr_endpoint_post() and there
we need to do len calculation as above *before* netdev_alloc_skb(NULL, len).
Perhaps there is a nicer solution though.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/qrtr: restrict length in qrtr_tun_write_iter()
2021-02-21 12:39 ` [PATCH] net/qrtr: restrict " Sabyrzhan Tasbolatov
@ 2021-02-22 8:45 ` Eric Dumazet
0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2021-02-22 8:45 UTC (permalink / raw)
To: Sabyrzhan Tasbolatov
Cc: davem, kuba, linux-kernel, netdev, syzbot+c2a7e5c5211605a90865
On 2/21/21 1:39 PM, Sabyrzhan Tasbolatov wrote:
>> Do we really expect to accept huge lengths here ?
>
> Sorry for late response but I couldnt find any reference to the max
> length of incoming data for qrtr TUN interface.
>
>> qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
>> some extra space (for struct skb_shared_info)
>
> Thanks, you're right, qrtr_endpoint_post() will alloc another slab buffer.
> We can check the length of skb allocation but we need to do following:
>
> int qrtr_endpoint_post(.., const void *data, size_t len)
> {
> ..
> when QRTR_PROTO_VER_1:
> hdrlen = sizeof(*data);
> when QRTR_PROTO_VER_2:
> hdrlen = sizeof(*data) + data->optlen;
>
> len = (KMALLOC_MAX_SIZE - hdrlen) % data->size;
> skb = netdev_alloc_skb(NULL, len);
> ..
> skb_put_data(skb, data + hdrlen, size);
>
>
> So it requires refactoring as in qrtr_tun_write_iter() we just allocate and
> pass it to qrtr_endpoint_post() and there
> we need to do len calculation as above *before* netdev_alloc_skb(NULL, len).
>
> Perhaps there is a nicer solution though.
>
A protocol requiring contiguous physical memory allocations of up to KMALLOC_MAX_SIZE
bytes would be really unreliable.
I suggest we simply limit the allocations to 64KB, unless qrtr maintainers shout,
or start implementing scatter gather.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-22 8:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 9:20 [PATCH] net/qrtr: restrict user-controlled length in qrtr_tun_write_iter() Sabyrzhan Tasbolatov
2021-02-04 0:28 ` Jakub Kicinski
2021-02-04 9:02 ` [PATCH] net/qrtr: replaced useless kzalloc with kmalloc " Sabyrzhan Tasbolatov
2021-02-04 18:51 ` Jakub Kicinski
2021-02-04 18:53 ` Jakub Kicinski
2021-02-04 0:40 ` [PATCH] net/qrtr: restrict user-controlled length " patchwork-bot+netdevbpf
2021-02-12 11:51 ` Eric Dumazet
2021-02-21 12:39 ` [PATCH] net/qrtr: restrict " Sabyrzhan Tasbolatov
2021-02-22 8:45 ` Eric Dumazet
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).