Re: [V9fs-developer] [PATCH] KASAN: slab-out-of-bounds Read in pdu_read
From: Dominique Martinet <asmadeus@codewreck.org>
Date: 2018-07-09 15:47:42
Also in:
lkml
Tomas Bortoli wrote on Mon, Jul 09, 2018:
I've analyzed this issue found by Syzkaller and I've made a patch:
Please use git send-email to send the patch, this is not applicable as is (partly because of the foreword you added (you can add arbitrary text or comment after the first --- in the patch itself or in a cover letter), but also because your mail client wrapped long lines so the patch itself is corrupted
The pdu_read() function suffers from an integer underflow. When pdu->offset is greater than pdu->size, the length calculation will have a wrong result, resulting in an out-of-bound read. Modify the pdu_write() function in the same way (as pdu_read()) although the modification is NOT necessary to fix crashes induced by the reproducer at issue I think it makes sense to have symmetry between the two functions (and a check more does not harm). The p9_client_version() does not initialize the version pointer. If the call to p9pdu_readf() returns an error and version has not been allocated in p9pdu_readf(), then the program will jump to the "error" label and will try to free the version pointer. If version is not initialized, free() might be called with uninitialized, garbage data and provoke a crash.
I'd suggest splitting the p9_client_version() change in a second patch as well, even if it's probably fine for this little.
Modify the p9_check_errors() function to check for PDUs with "size > capacity" to prevent out-of-bound reads.
Ditto as it's unrelated as well.
size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
{
- size_t len = min(pdu->size - pdu->offset, size);
- memcpy(data, &pdu->sdata[pdu->offset], len);
+ size_t len = pdu->offset > pdu->size ? 0 : min(pdu->size -
pdu->offset, size);This line (once unwrapped) is over 80 characters, try to run checkpatch.pl against your patch
quoted hunk
+ if(len != 0) + memcpy(data, &pdu->sdata[pdu->offset], len); pdu->offset += len; return size - len; } static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size) { - size_t len = min(pdu->capacity - pdu->size, size); - memcpy(&pdu->sdata[pdu->size], data, len); + size_t len = pdu->size > pdu->capacity ? 0 : min(pdu->capacity - pdu->size, size); + if(len != 0) + memcpy(&pdu->sdata[pdu->size], data, len); pdu->size += len; return size - len; }--- b/net/9p/client.c 2018-07-09 16:46:25.459541292 +0200 +++ a/net/9p/client.c 2018-07-09 16:15:36.337500567 +0200@@ -519,10 +519,13 @@ EXPORT_SYMBOL(p9_parse_header);static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) { int8_t type; + int32_t size; int err; int ecode; err = p9_parse_header(req->rc, &size, &type, NULL, 0);
Your patch adds the 'size' variable a few lines above but this line didn't change? Something is wrong here; you changed a NULL to &size so git should pick it up.
+ if(size > req->rc->capacity) + return -EINVAL;
Hmm, tricky - this isn't enough, we could read uninitialized data if size is bigger than what was read but still < capacity. For trans_fd, rc->offset seems to hold the size actually read but other transports do not do this properly. This might work as a band-aid but it needs a bigger patch that properly stores in the fcall how much was read and then checks that. trans_rdma does not store the actual read size at all (it is in the 'wc->byte_len' in 'recv_done()'), it doesn't look like virtio does it either but I am not familiar with that part of the code.
Looking forward your feedback,
Thanks for taking the time to look at the syzbot reports and send a patch, do not hesitate to ask questions if I wasn't clear or if you need help with the bigger change I suggested. -- Dominique Martinet | Asmadeus