Re: [V9fs-developer] [PATCH] KASAN: slab-out-of-bounds Read in pdu_read

Re: [V9fs-developer] [PATCH] KASAN: slab-out-of-bounds Read in pdu_read

[Dominique Martinet]

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

> +    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