[PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[GUO Zihua]

This patchset fixes the write overflow issue in p9_read_work. As well as
some follow up cleanups.

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>

GUO Zihua (3):
  9p: Fix write overflow in p9_read_work
  9p: Remove redundent checks for message size against msize.
  9p: Use P9_HDRSZ for header size

 net/9p/trans_fd.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

---

v2:
  Addition log for debugging similar issues, as well as cleanups according to
  Dominique's comment.
-- 
2.17.1

[PATCH 1/3 v2] 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, it
turns out that capacity is no longer consistent with msize. Thus,
the message size should also be checked against sdata capacity.

Reported-by: syzbot+0f89bd13eaceccc0e126@syzkaller.appspotmail.com
Fixes: 60ece0833b6c ("net/9p: allocate appropriate reduced message buffers")
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..30f37bf7c598 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 for tag %d with capacity %zd\n",
+				 m->rc.size, m->rc.tag, m->rreq->rc.capacity);
+			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

[PATCH 2/3 v2] 9p: Remove redundent checks for message size against msize.

[GUO Zihua]

As the msize is non-consistant with the capacity of the tag and as we
are now checking message size against capacity directly, there is no
point checking message size against msize. So remove it.

Fixes: 3da2e34b64cd ("9p: Fix write overflow in p9_read_work")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 net/9p/trans_fd.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 30f37bf7c598..4ba602438550 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -322,14 +322,6 @@ static void p9_read_work(struct work_struct *work)
 			goto error;
 		}
 
-		if (m->rc.size >= m->client->msize) {
-			p9_debug(P9_DEBUG_ERROR,
-				 "requested packet size too big: %d\n",
-				 m->rc.size);
-			err = -EIO;
-			goto error;
-		}
-
 		p9_debug(P9_DEBUG_TRANS,
 			 "mux %p pkt: size: %d bytes tag: %d\n",
 			 m, m->rc.size, m->rc.tag);
-- 
2.17.1

[PATCH 3/3 v2] 9p: Use P9_HDRSZ for header size

[GUO Zihua]

The m->rc.offset here actually represents the header size of a p9
message. So instead we use P9_HDRSZ directly. At the mean time, update
all header sizes as well.

Fixes: 3da2e34b64cd ("9p: Fix write overflow in p9_read_work")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 net/9p/trans_fd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 4ba602438550..89a51fcc831d 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -120,7 +120,7 @@ struct p9_conn {
 	struct list_head unsent_req_list;
 	struct p9_req_t *rreq;
 	struct p9_req_t *wreq;
-	char tmp_buf[7];
+	char tmp_buf[P9_HDRSZ];
 	struct p9_fcall rc;
 	int wpos;
 	int wsize;
@@ -291,7 +291,7 @@ static void p9_read_work(struct work_struct *work)
 	if (!m->rc.sdata) {
 		m->rc.sdata = m->tmp_buf;
 		m->rc.offset = 0;
-		m->rc.capacity = 7; /* start by reading header */
+		m->rc.capacity = P9_HDRSZ; /* start by reading header */
 	}
 
 	clear_bit(Rpending, &m->wsched);
@@ -314,7 +314,7 @@ static void p9_read_work(struct work_struct *work)
 		p9_debug(P9_DEBUG_TRANS, "got new header\n");
 
 		/* Header size */
-		m->rc.size = 7;
+		m->rc.size = P9_HDRSZ;
 		err = p9_parse_header(&m->rc, &m->rc.size, NULL, NULL, 0);
 		if (err) {
 			p9_debug(P9_DEBUG_ERROR,
@@ -334,7 +334,7 @@ static void p9_read_work(struct work_struct *work)
 			goto error;
 		}
 
-		if (m->rc.size > m->rreq->rc.capacity - m->rc.offset) {
+		if (m->rc.size > m->rreq->rc.capacity - P9_HDRSZ) {
 			p9_debug(P9_DEBUG_ERROR,
 				 "requested packet size too big: %d for tag %d with capacity %zd\n",
 				 m->rc.size, m->rc.tag, m->rreq->rc.capacity);
-- 
2.17.1

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[asmadeus]

GUO Zihua wrote on Thu, Nov 17, 2022 at 05:11:56PM +0800:
> This patchset fixes the write overflow issue in p9_read_work. As well as
> some follow up cleanups.

Thanks for this v2.

Comments below

> GUO Zihua (3):
>   9p: Fix write overflow in p9_read_work
>   9p: Remove redundent checks for message size against msize.

This has 'Fixes: 3da2e34b64cd ("9p: Fix write overflow in
p9_read_work")' but that commit isn't applied yet, so the commit hash
only exists in your tree -- I will get a different hash when I apply the
patch (because it'll contain my name as committer, date changed etc)

I don't think it really makes sense to separate these two patches, I'll
squash them together on my side.

>   9p: Use P9_HDRSZ for header size

This makes sense to keep separate, I'll just drop the 'fixes' tag for
the same reason as above


I'll do the squash & test tomorrow, you don't need to resend.
I will tell you when I push to next so you can check you're happy with
my version.
-- 
Dominique

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[Guozihua (Scott)]

On 2022/11/17 18:49, asmadeus@codewreck.org wrote:
> GUO Zihua wrote on Thu, Nov 17, 2022 at 05:11:56PM +0800:
>> This patchset fixes the write overflow issue in p9_read_work. As well as
>> some follow up cleanups.
> 
> Thanks for this v2.
> 
> Comments below
> 
>> GUO Zihua (3):
>>    9p: Fix write overflow in p9_read_work
>>    9p: Remove redundent checks for message size against msize.
> 
> This has 'Fixes: 3da2e34b64cd ("9p: Fix write overflow in
> p9_read_work")' but that commit isn't applied yet, so the commit hash
> only exists in your tree -- I will get a different hash when I apply the
> patch (because it'll contain my name as committer, date changed etc)
> 
> I don't think it really makes sense to separate these two patches, I'll
> squash them together on my side.
> 
>>    9p: Use P9_HDRSZ for header size
> 
> This makes sense to keep separate, I'll just drop the 'fixes' tag for
> the same reason as above
> 
> 
> I'll do the squash & test tomorrow, you don't need to resend.
> I will tell you when I push to next so you can check you're happy with
> my version.

Sounds great! Thanks Dominique!
-- 
Best
GUO Zihua

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[Christian Schoenebeck]

On Thursday, November 17, 2022 10:11:56 AM CET GUO Zihua wrote:
> This patchset fixes the write overflow issue in p9_read_work. As well as
> some follow up cleanups.
> 
> 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>
> 
> GUO Zihua (3):
>   9p: Fix write overflow in p9_read_work
>   9p: Remove redundent checks for message size against msize.
>   9p: Use P9_HDRSZ for header size

For entire series:

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

I agree with Dominique that patch 1 and 2 should be merged.

>  net/9p/trans_fd.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> ---
> 
> v2:
>   Addition log for debugging similar issues, as well as cleanups according to
>   Dominique's comment.
> 

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[asmadeus]

Christian Schoenebeck wrote on Thu, Nov 17, 2022 at 02:33:28PM +0100:
> > GUO Zihua (3):
> >   9p: Fix write overflow in p9_read_work
> >   9p: Remove redundent checks for message size against msize.
> >   9p: Use P9_HDRSZ for header size
> 
> For entire series:
> 
> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> 
> I agree with Dominique that patch 1 and 2 should be merged.

Thank you both!

I've just pushed the patches to my next branch:
https://github.com/martinetd/linux/commits/9p-next

... with a twist, as the original patch fails on any normal workload:
---
/ # ls /mnt
9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11
---
(so much for having two pairs of eyes :-D
By the way we -really- need to replace P9_DEBUG_ERROR by pr_error or
something, these should be displayed without having to specify
debug=1...)


capacity is only set in a handful of places (alloc time, hardcoded 7 in
trans_fd, after receiving packet) so I've added logs and our alloc
really passed '11' for alloc_rsize (logging tsize/rsize)

9pnet: (00000087) >>> TWALK fids 1,2 nwname 0d wname[0] (null)
9pnet: -- p9_tag_alloc (87): allocating capacity to 17/11 for tag 0
9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11

... So this is RWALK, right:
size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13])
4 ..... 5.... 7..... 9....... packet end at 9 as 0 nwqid.
We have capacity 11 to allow rlerror_size which is bigger; everything is
good.

Long story short, the size header includes the header size, when I
misread https://9fans.github.io/plan9port/man/man9/version.html to
say it didn't (it just says it doesn't include the enveloping transport
protocol, it starts from size alright and I just misread that)
Thanksfully the code caught it.

So I've just removed the - offset part and things appear to work again.

Guo Zihua, can you check this still fixes your syz repro, or was that
substraction needed? If it's still needed we have an off by 1 somewhere
to look for.

-- 
Dominique

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[Guozihua (Scott)]

On 2022/11/18 12:59, asmadeus@codewreck.org wrote:
> Christian Schoenebeck wrote on Thu, Nov 17, 2022 at 02:33:28PM +0100:
>>> GUO Zihua (3):
>>>    9p: Fix write overflow in p9_read_work
>>>    9p: Remove redundent checks for message size against msize.
>>>    9p: Use P9_HDRSZ for header size
>>
>> For entire series:
>>
>> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
>>
>> I agree with Dominique that patch 1 and 2 should be merged.
> 
> Thank you both!
> 
> I've just pushed the patches to my next branch:
> https://github.com/martinetd/linux/commits/9p-next
> 
> ... with a twist, as the original patch fails on any normal workload:
> ---
> / # ls /mnt
> 9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11
> ---
> (so much for having two pairs of eyes :-D
> By the way we -really- need to replace P9_DEBUG_ERROR by pr_error or
> something, these should be displayed without having to specify
> debug=1...)
> 
> 
> capacity is only set in a handful of places (alloc time, hardcoded 7 in
> trans_fd, after receiving packet) so I've added logs and our alloc
> really passed '11' for alloc_rsize (logging tsize/rsize)
> 
> 9pnet: (00000087) >>> TWALK fids 1,2 nwname 0d wname[0] (null)
> 9pnet: -- p9_tag_alloc (87): allocating capacity to 17/11 for tag 0
> 9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11
> 
> ... So this is RWALK, right:
> size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13])
> 4 ..... 5.... 7..... 9....... packet end at 9 as 0 nwqid.
> We have capacity 11 to allow rlerror_size which is bigger; everything is
> good.
> 
> Long story short, the size header includes the header size, when I
> misread https://9fans.github.io/plan9port/man/man9/version.html to
> say it didn't (it just says it doesn't include the enveloping transport
> protocol, it starts from size alright and I just misread that)
> Thanksfully the code caught it.
> 
> So I've just removed the - offset part and things appear to work again.
> 
> Guo Zihua, can you check this still fixes your syz repro, or was that
> substraction needed? If it's still needed we have an off by 1 somewhere
> to look for.
> 

Hi Dominique, I retried the repro on your branch, the issue does not 
reproduce. What a good pair of eyes :)！

-- 
Best
GUO Zihua

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[asmadeus]

Guozihua (Scott) wrote on Fri, Nov 18, 2022 at 06:18:16PM +0800:
> I retried the repro on your branch, the issue does not reproduce. What
> a good pair of eyes :)！

Thanks!
By the way the original check also compared size to msize directly,
without an offset for headers, so with hindsight it looks clear enough
that the size is the full size including the header.

I'm not sure why I convinced myself it didn't...

Anyway, this made me check other places where we might fail at this and
I've a couple more patches; please review if you have time.
I'll send them all to Linus next week...
-- 
Dominique

Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

[Christian Schoenebeck]

On Friday, November 18, 2022 2:57:14 PM CET asmadeus@codewreck.org wrote:
> Guozihua (Scott) wrote on Fri, Nov 18, 2022 at 06:18:16PM +0800:
> > I retried the repro on your branch, the issue does not reproduce. What
> > a good pair of eyes :)！
> 
> Thanks!
> By the way the original check also compared size to msize directly,
> without an offset for headers, so with hindsight it looks clear enough
> that the size is the full size including the header.
> 
> I'm not sure why I convinced myself it didn't...
> 
> Anyway, this made me check other places where we might fail at this and
> I've a couple more patches; please review if you have time.
> I'll send them all to Linus next week...
> 

Aah, the offset is already incremented before that block is entered:

303	err = p9_fd_read(m->client, m->rc.sdata + m->rc.offset,
304			 m->rc.capacity - m->rc.offset);
...
312	m->rc.offset += err;
313
314	/* header read in */
315	if ((!m->rreq) && (m->rc.offset == m->rc.capacity)) {

And the data is then copied to m->rreq->rc.sdata without any offset. So yes,
there should be no `offset` in the check.

Best regards,
Christian Schoenebeck