[PATCH] 9p: Fix write overflow in p9_read_work

[PATCH] 9p: Fix write overflow in p9_read_work

[GUO Zihua]

This error was reported while fuzzing:

BUG: KASAN: slab-out-of-bounds in _copy_to_iter+0xd35/0x1190
Write of size 4043 at addr ffff888008724eb1 by task kworker/1:1/24

CPU: 1 PID: 24 Comm: kworker/1:1 Not tainted 6.1.0-rc5-00002-g1adf73218daa-dirty #223
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
Workqueue: events p9_read_work
Call Trace:
 <TASK>
 dump_stack_lvl+0x4c/0x64
 print_report+0x178/0x4b0
 kasan_report+0xae/0x130
 kasan_check_range+0x179/0x1e0
 memcpy+0x38/0x60
 _copy_to_iter+0xd35/0x1190
 copy_page_to_iter+0x1d5/0xb00
 pipe_read+0x3a1/0xd90
 __kernel_read+0x2a5/0x760
 kernel_read+0x47/0x60
 p9_read_work+0x463/0x780
 process_one_work+0x91d/0x1300
 worker_thread+0x8c/0x1210
 kthread+0x280/0x330
 ret_from_fork+0x22/0x30
 </TASK>

Allocated by task 457:
 kasan_save_stack+0x1c/0x40
 kasan_set_track+0x21/0x30
 __kasan_kmalloc+0x7e/0x90
 __kmalloc+0x59/0x140
 p9_fcall_init.isra.11+0x5d/0x1c0
 p9_tag_alloc+0x251/0x550
 p9_client_prepare_req+0x162/0x350
 p9_client_rpc+0x18d/0xa90
 p9_client_create+0x670/0x14e0
 v9fs_session_init+0x1fd/0x14f0
 v9fs_mount+0xd7/0xaf0
 legacy_get_tree+0xf3/0x1f0
 vfs_get_tree+0x86/0x2c0
 path_mount+0x885/0x1940
 do_mount+0xec/0x100
 __x64_sys_mount+0x1a0/0x1e0
 do_syscall_64+0x3a/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

This BUG pops up when trying to reproduce
https://syzkaller.appspot.com/bug?id=6c7cd46c7bdd0e86f95d26ec3153208ad186f9fa.
The callstack is different but the issue is valid and re-producable with
the same re-producer in the link.

The root cause of this issue is that we check the size of the message
received against the msize of the client in p9_read_work. However, this
msize could be lager than the capacity of the sdata buffer. Thus,
the message size should also be checked against sdata capacity.

Reported-by: syzbot+0f89bd13eaceccc0e126@syzkaller.appspotmail.com
Fixes: 1b0a763bdd5e ("9p: use the rcall structure passed in the request in trans_fd read_work")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 net/9p/trans_fd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 56a186768750..bc131a21c098 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -342,6 +342,14 @@ static void p9_read_work(struct work_struct *work)
 			goto error;
 		}
 
+		if (m->rc.size > m->rreq->rc.capacity - m->rc.offset) {
+			p9_debug(P9_DEBUG_ERROR,
+				 "requested packet size too big: %d\n",
+				 m->rc.size);
+			err = -EIO;
+			goto error;
+		}
+
 		if (!m->rreq->rc.sdata) {
 			p9_debug(P9_DEBUG_ERROR,
 				 "No recv fcall for tag %d (req %p), disconnecting!\n",
-- 
2.17.1

Re: [PATCH] 9p: Fix write overflow in p9_read_work

[asmadeus]

GUO Zihua wrote on Thu, Nov 17, 2022 at 02:14:44PM +0800:
> The root cause of this issue is that we check the size of the message
> received against the msize of the client in p9_read_work. However, this
> msize could be lager than the capacity of the sdata buffer. Thus,
> the message size should also be checked against sdata capacity.

Thanks for the fix!

I'm picky, so a few remarks below.

> 
> Reported-by: syzbot+0f89bd13eaceccc0e126@syzkaller.appspotmail.com
> Fixes: 1b0a763bdd5e ("9p: use the rcall structure passed in the request in trans_fd read_work")
> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> ---
>  net/9p/trans_fd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 56a186768750..bc131a21c098 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -342,6 +342,14 @@ static void p9_read_work(struct work_struct *work)
>  			goto error;
>  		}
>  
> +		if (m->rc.size > m->rreq->rc.capacity - m->rc.offset) {

Ah, it took me a while to understand but capacity here is no longer the
same as msize since commit 60ece0833b6c ("net/9p: allocate appropriate
reduced message buffers")

If you have time to test the reproducer, please check with any commit
before 60ece0833b6c if you can still reproduce. If not please fix your
Fixes tag to this commit.
I'd appreciate a word in the commit message saying that message capacity
is no longer constant here and needs a more subtle check than msize.


Also:
 - We can remove the msize check, it's redundant with this; it doesn't
matter if we don't check for msize before doing the tag lookup as tag
has already been read
 - While the `- offset` part of the check is correct (rc.size does
not include headers, and the current offset must be 7 here) I'd prefer
if you woud use P9_HDRSZ as that's defined in the protocol and using
macros will be easier to check if that ever evolves.
 - (I'd also appreciate if you could update the capacity = 7 next to the
'start by reading header' comment above while you're here so we use the
same macro in both place)


> +			p9_debug(P9_DEBUG_ERROR,
> +				 "requested packet size too big: %d\n",
> +				 m->rc.size);

Please log m->rc.tag, m->rc.id and m->rreq->rc.capacity as well for
debugging if that happens.

> +			err = -EIO;
> +			goto error;
> +		}
> +
>  		if (!m->rreq->rc.sdata) {
>  			p9_debug(P9_DEBUG_ERROR,
> 				 "No recv fcall for tag %d (req %p), disconnecting!\n",
--
Dominique

Re: [PATCH] 9p: Fix write overflow in p9_read_work

[Guozihua (Scott)]

On 2022/11/17 15:29, asmadeus@codewreck.org wrote:
> GUO Zihua wrote on Thu, Nov 17, 2022 at 02:14:44PM +0800:
>> The root cause of this issue is that we check the size of the message
>> received against the msize of the client in p9_read_work. However, this
>> msize could be lager than the capacity of the sdata buffer. Thus,
>> the message size should also be checked against sdata capacity.
> 
> Thanks for the fix!
> 
> I'm picky, so a few remarks below.
> 
>>
>> Reported-by: syzbot+0f89bd13eaceccc0e126@syzkaller.appspotmail.com
>> Fixes: 1b0a763bdd5e ("9p: use the rcall structure passed in the request in trans_fd read_work")
>> Signed-off-by: GUO Zihua <guozihua@huawei.com>
>> ---
>>   net/9p/trans_fd.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
>> index 56a186768750..bc131a21c098 100644
>> --- a/net/9p/trans_fd.c
>> +++ b/net/9p/trans_fd.c
>> @@ -342,6 +342,14 @@ static void p9_read_work(struct work_struct *work)
>>   			goto error;
>>   		}
>>   
>> +		if (m->rc.size > m->rreq->rc.capacity - m->rc.offset) {
> 
> Ah, it took me a while to understand but capacity here is no longer the
> same as msize since commit 60ece0833b6c ("net/9p: allocate appropriate
> reduced message buffers")
> 
> If you have time to test the reproducer, please check with any commit
> before 60ece0833b6c if you can still reproduce. If not please fix your
> Fixes tag to this commit.
> I'd appreciate a word in the commit message saying that message capacity
> is no longer constant here and needs a more subtle check than msize.
> 
> 
> Also:
>   - We can remove the msize check, it's redundant with this; it doesn't
> matter if we don't check for msize before doing the tag lookup as tag
> has already been read
>   - While the `- offset` part of the check is correct (rc.size does
> not include headers, and the current offset must be 7 here) I'd prefer
> if you woud use P9_HDRSZ as that's defined in the protocol and using
> macros will be easier to check if that ever evolves.
>   - (I'd also appreciate if you could update the capacity = 7 next to the
> 'start by reading header' comment above while you're here so we use the
> same macro in both place)
> 
> 
>> +			p9_debug(P9_DEBUG_ERROR,
>> +				 "requested packet size too big: %d\n",
>> +				 m->rc.size);
> 
> Please log m->rc.tag, m->rc.id and m->rreq->rc.capacity as well for
> debugging if that happens.
> 
>> +			err = -EIO;
>> +			goto error;
>> +		}
>> +
>>   		if (!m->rreq->rc.sdata) {
>>   			p9_debug(P9_DEBUG_ERROR,
>> 				 "No recv fcall for tag %d (req %p), disconnecting!\n",
> --
> Dominique

Hi Dominique, Thanks for the comment, will push a v2 right away.
-- 
Best
GUO Zihua