[PATCH] 9p: use unsinged integers for nwqid/count

[PATCH] 9p: use unsinged integers for nwqid/count

[Kirill A. Shutemov]

As specification says, all integers in messages are unsigned. Let's fix
behaviour of p9pdu_vreadf()/p9pdu_vwritef() accordingly.

Fix for p9pdu_vreadf() is critical. If server replies with Rwalk, where
nwqid > SHRT_MAX, the value will be interpreted as negative. kmalloc, in
its order, will cast the value to (very big) size_t:

[   16.330386] ------------[ cut here ]------------
[   16.331479] WARNING: CPU: 10 PID: 239 at /home/kas/git/public/linux-next/mm/page_alloc.c:2660 __alloc_pages_nodemask+0x4c9/0xb80()
[   16.333039] Modules linked in:
[   16.333433] CPU: 10 PID: 239 Comm: trinity-c62 Not tainted 3.19.0-rc1-next-20141226-00040-g0bb79cfd1d6c-dirty #388
[   16.334804] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.7.5.1-0-g8936dbb-20141113_115728-nilsson.home.kraxel.org 04/01/2014
[   16.337407]  ffffffff81c7e8e8 ffff88046bf77978 ffffffff81854c3a 0000000000000c52
[   16.339154]  0000000000000000 ffff88046bf779b8 ffffffff8105adda 0000000000000b3c
[   16.340702]  0000000000000000 ffff880495f0df80 0000000000000000 ffff880495f0ef00
[   16.342414] Call Trace:
[   16.342996]  [<ffffffff81854c3a>] dump_stack+0x4c/0x65
[   16.344129]  [<ffffffff8105adda>] warn_slowpath_common+0x8a/0xc0
[   16.345312]  [<ffffffff8105aeca>] warn_slowpath_null+0x1a/0x20
[   16.346644]  [<ffffffff811560c9>] __alloc_pages_nodemask+0x4c9/0xb80
[   16.347876]  [<ffffffff81092385>] ? sched_clock_local+0x25/0x90
[   16.348896]  [<ffffffff81086a57>] ? finish_task_switch+0x87/0x110
[   16.350106]  [<ffffffff8119ffbb>] alloc_pages_current+0x6b/0xc0
[   16.351299]  [<ffffffff81151ade>] alloc_kmem_pages+0xe/0x10
[   16.352293]  [<ffffffff81173458>] kmalloc_order+0x18/0x50
[   16.353348]  [<ffffffff811734b4>] kmalloc_order_trace+0x24/0x210
[   16.354528]  [<ffffffff811ad9e9>] __kmalloc+0x2b9/0x3b0
[   16.355623]  [<ffffffff81841daa>] p9pdu_vreadf+0x57a/0x9b0
[   16.356392]  [<ffffffff810a84ce>] ? __lock_is_held+0x5e/0x90
[   16.357142]  [<ffffffff8183b4c3>] ? p9_client_rpc+0x813/0x840
[   16.358025]  [<ffffffff81841829>] p9pdu_readf+0x39/0x40
[   16.358780]  [<ffffffff8183ede2>] p9_client_walk+0xc2/0x400
[   16.359699]  [<ffffffff811adab8>] ? __kmalloc+0x388/0x3b0
[   16.360755]  [<ffffffff8139eb75>] ? v9fs_fid_lookup_with_uid+0x195/0x310
[   16.361123] VFS: Warning: trinity-c56 using old stat() call. Recompile your binary.
[   16.363903]  [<ffffffff8139ec0f>] v9fs_fid_lookup_with_uid+0x22f/0x310
[   16.365422]  [<ffffffff8139ed5b>] v9fs_fid_lookup+0x6b/0x80
[   16.366815]  [<ffffffff813985ab>] v9fs_statfs+0x1b/0xc0
[   16.368185]  [<ffffffff811f6c82>] statfs_by_dentry+0x62/0x90
[   16.369634]  [<ffffffff811f6e4b>] vfs_statfs+0x1b/0xb0
[   16.370951]  [<ffffffff811f6fa3>] fd_statfs+0x33/0x60
[   16.372075]  [<ffffffff811f7090>] SyS_fstatfs+0x20/0x40
[   16.373362]  [<ffffffff8185e2b7>] ? sysret_check+0x1b/0x56
[   16.374576]  [<ffffffff8185e292>] system_call_fastpath+0x12/0x17
[   16.375815] ---[ end trace 949914f1bb21cfd4 ]---

It should never happen in normal situation: we never submit Twalk with
nwname > 16, but malicious or broken server can still produce
problematic Rwalk.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 net/9p/protocol.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index ab9127ec5b7a..305e4789f2cc 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -273,7 +273,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt,
 			}
 			break;
 		case 'R':{
-				int16_t *nwqid = va_arg(ap, int16_t *);
+				uint16_t *nwqid = va_arg(ap, uint16_t *);
 				struct p9_qid **wqids =
 				    va_arg(ap, struct p9_qid **);
 
@@ -448,7 +448,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
 			}
 			break;
 		case 'U':{
-				int32_t count = va_arg(ap, int32_t);
+				uint32_t count = va_arg(ap, uint32_t);
 				const char __user *udata =
 						va_arg(ap, const void __user *);
 				errcode = p9pdu_writef(pdu, proto_version, "d",
@@ -479,7 +479,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
 			}
 			break;
 		case 'R':{
-				int16_t nwqid = va_arg(ap, int);
+				uint16_t nwqid = va_arg(ap, int);
 				struct p9_qid *wqids =
 				    va_arg(ap, struct p9_qid *);
 
-- 
2.1.4

Re: [V9fs-developer] [PATCH] 9p: use unsinged integers for nwqid/count

[Dominique Martinet]

Hi,

Kirill A. Shutemov wrote on Tue, Dec 30, 2014 at 02:48:09AM +0200:
> @@ -273,7 +273,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt,
>  			}
>  			break;
>  		case 'R':{
> -				int16_t *nwqid = va_arg(ap, int16_t *);
> +				uint16_t *nwqid = va_arg(ap, uint16_t *);
>  				struct p9_qid **wqids =
>  				    va_arg(ap, struct p9_qid **);
>  

Good find there!

Given we also have pdu->size, would it make sense to check nwqid through
this?
I'd need to check but I'd assume we should always have, after reading
nwqid, pdu->size - pdu->offset >= *nwqid * 13 (size of qid on the wire)

If not, one of the p9pdu_readf for qids is bound to fail eventually, and
there has been a problem somewhere.
(It should be equal for walk because this ends the pdu, but nothing
forbids a protocol extension that'd add more data after this qid array)


> @@ -448,7 +448,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>  			}
>  			break;
>  		case 'U':{
> -				int32_t count = va_arg(ap, int32_t);
> +				uint32_t count = va_arg(ap, uint32_t);
>  				const char __user *udata =
>  						va_arg(ap, const void __user *);
>  				errcode = p9pdu_writef(pdu, proto_version, "d",
> @@ -479,7 +479,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>  			}
>  			break;
>  		case 'R':{
> -				int16_t nwqid = va_arg(ap, int);
> +				uint16_t nwqid = va_arg(ap, int);
>  				struct p9_qid *wqids =
>  				    va_arg(ap, struct p9_qid *);

Doesn't seem like it can hurt, although I can't see any code leading to
the 'R' case. It's a bit hard to tell.

Anyway, supporting this patch as well, second double-check on read/nwqid
can be added separately if deemed useful!

-- 
Dominique

Re: [V9fs-developer] [PATCH] 9p: use unsinged integers for nwqid/count

[Kirill A. Shutemov]

On Tue, Jan 06, 2015 at 02:04:02PM +0100, Dominique Martinet wrote:
> Hi,
> 
> Kirill A. Shutemov wrote on Tue, Dec 30, 2014 at 02:48:09AM +0200:
> > @@ -273,7 +273,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt,
> >  			}
> >  			break;
> >  		case 'R':{
> > -				int16_t *nwqid = va_arg(ap, int16_t *);
> > +				uint16_t *nwqid = va_arg(ap, uint16_t *);
> >  				struct p9_qid **wqids =
> >  				    va_arg(ap, struct p9_qid **);
> >  
> 
> Good find there!
> 
> Given we also have pdu->size, would it make sense to check nwqid through
> this?
> I'd need to check but I'd assume we should always have, after reading
> nwqid, pdu->size - pdu->offset >= *nwqid * 13 (size of qid on the wire)

Other option is to ask caller for upper limit. Since it's reply to Twalk
request, we should never see nwqid > nwname on request.

-- 
 Kirill A. Shutemov