WARNING in kmem_cache_create_usercopy

RIP: 0010:kmem_cache_create_usercopy+0xad/0x240 mm/slab_common.c:473
Code: 44 89 f0 25 00 60 de 04 45 85 ed 89 45 cc 75 0b 8b 45 d0 85 c0

0f 85 8e 01 00 00 44 39 eb 72 0a 89 d8 44 29 e8 3b 45 d0 73 7e <0f> 0b c7 45 d0 00 00 00 00 4c 8b 45 10 44 89 fa 89 de 4c 89 e7 8b
RSP: 0018:ffff8801bc23f5d0 EFLAGS: 00010213
RAX: 0000000000000000 RBX: 0000000000000008 RCX: 0000000000000006
RDX: 0000000000000000 RSI: 0000000000000020 RDI: ffffffff88b04b20
RBP: ffff8801bc23f608 R08: fffffbfff1283a2d R09: fffffbfff1283a2c
R10: ffff8801bc23f5c0 R11: ffffffff8941d167 R12: ffffffff88b04b20
R13: 00000000fffffffd R14: 0000000000000000 R15: 0000000000000000
p9_client_create+0xa58/0x1769 net/9p/client.c:1054

From: Dominique Martinet <[email protected]>

If the requested msize is too small (either from command line argument or from the server version reply), we won't get any work done. If it's *really* too small, nothing will work, and this got caught by syzbot recently (on a new kmem_cache_create_usercopy() call)

Just set a minimum msize to 4k in both code paths, until someone complains they have a use-case for a smaller msize.

We need to check in both mount option and server reply individually because the msize for the first version request would be unchecked with just a global check on clnt->msize.

Reported-by [email protected]

Signed-off-by Dominique Martinet <[email protected]>

Cc Eric Van Hensbergen <[email protected]>

Cc Latchesar Ionkov <[email protected]>

---
 net/9p/client.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

net/9p/client.c +0 -0

index 2c9a17b9b46b..b1163ebdc622 100644

net/9p/client.c +20 -0

+				p9_debug(P9_DEBUG_ERROR,
+					 "msize should be at least 4k\n");
+				ret = -EINVAL;
+				continue;
+			}
 			clnt->msize = option;
 			break;
 		case Opt_trans:

@@ -983,10 +989,18 @@ static int p9_client_version(struct p9_client *c)
 	else if (!strncmp(version, "9P2000", 6))
 		c->proto_version = p9_proto_legacy;
 	else {
+		p9_debug(P9_DEBUG_ERROR,
+			 "server returned an unknown version: %s\n", version);
 		err = -EREMOTEIO;
 		goto error;
 	}
 
+	if (msize < 4096) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "server returned a msize < 4096: %d\n", msize);
+		err = -EREMOTEIO;
+		goto error;
+	}
 	if (msize < c->msize)
 		c->msize = msize;
 

@@ -1043,6 +1057,13 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	if (clnt->msize > clnt->trans_mod->maxsize)
 		clnt->msize = clnt->trans_mod->maxsize;
 
+	if (clnt->msize < 4096) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "Please specify a msize of at least 4k\n");
+		err = -EINVAL;
+		goto free_client;
+	}
+
 	err = p9_client_version(clnt);
 	if (err)
 		goto close_trans;

syzbot wrote on Fri, Nov 02, 2018:

Quoted 11 lines

RIP: 0010:kmem_cache_create_usercopy+0xad/0x240 mm/slab_common.c:473
Code: 44 89 f0 25 00 60 de 04 45 85 ed 89 45 cc 75 0b 8b 45 d0 85 c0

0f 85 8e 01 00 00 44 39 eb 72 0a 89 d8 44 29 e8 3b 45 d0 73 7e <0f> 0b c7 45 d0 00 00 00 00 4c 8b 45 10 44 89 fa 89 de 4c 89 e7 8b
RSP: 0018:ffff8801bc23f5d0 EFLAGS: 00010213
RAX: 0000000000000000 RBX: 0000000000000008 RCX: 0000000000000006
RDX: 0000000000000000 RSI: 0000000000000020 RDI: ffffffff88b04b20
RBP: ffff8801bc23f608 R08: fffffbfff1283a2d R09: fffffbfff1283a2c
R10: ffff8801bc23f5c0 R11: ffffffff8941d167 R12: ffffffff88b04b20
R13: 00000000fffffffd R14: 0000000000000000 R15: 0000000000000000
p9_client_create+0xa58/0x1769 net/9p/client.c:1054

No lower bound on msize, the reproducer gives a reply from the pseudo-server with msize=8 and we happily take it, underflowing the msize - 11 (hdr+4) argument to kmem_cache_create_usercopy... This probably never worked anyway, but we now get a warning :)

We need to add a sane lower bound to msize as well as the current upper bound set in the transport.
We have some header sizes in 9p.h for IO header overhead (P9_IOHDRSZ to 24 for example) but I think that's too low in practice; stuff like readdir will require more than this to get a single entry... We can request the server to ask for at least 4k?
9p would probably work with less (e.g. 1k; I'd rather not have to figgure the minimum length we need to get each messages to work in its minimal form) but honestly even with 4k the perforamnces will be terrible, so tempted to go with that...

I'll send a patch imposing 4k next week unless someone else does first, or replies indicate different preferences.

@Dmitry: semi-related, the C reproducer (
https://syzkaller.appspot.com/x/repro.c?x=1701f831400000 ) has a lot of "readable" letters spelled out as "\x63..." or chars as 0x3d - it's fine for generated code and that might be easier for the intermediate representation syzkaller works with, but do you know something handy that would help convert these to readable strings?
e.g. "\x63\x61\x63\x68\x65\x3d\xc0\x6d\x61\x70" could be written "cache=\xc0map", and 0x3d as '=' (hm I guess the later would not always make sense to convert so probably best left as is, but it gets annoying pretty fast with longer strings)