On Thu, Jul 12, 2018 at 03:32:56PM +0300, Dan Carpenter wrote:
> "dcbuf" is a union that is "size" bytes large. We ensure that "nbytes"
> is large enough to hold the smallest member of the union, but if we
> require a larger union member then then we could access beyond the end
> of the allocated memory in coda_downcall().
I don't see where nbytes is set to hold the smallest member of the
union.
// nbytes is how much userspace is trying to write
union outputArgs *dcbuf;
int size = sizeof(*dcbuf); // maximum size of the union
...
if (nbytes > size) {
...
nbytes = size; // truncate nbytes if the write was
// larger than our buffer
}
CODA_ALLOC(*dcbuf, union outputArgs *, nbytes);
copy_from_user(dcbuf, buf, nbytes);
The test between the sizeof and the truncation of nbytes in the
ellipsized part of the code does test for the smallest size of the
union, but it has a 'goto out;' when it is hit because if the received
message is smaller than the message header, the code that would run
after the copy_from_user would look at fields that were never passed by
userspace.
Even if we allocate size instead of nbytes, we still wouldn't
copy_from_user more than nbytes anyway.
Jan
> The union is quite small so we can allocate enough space so everything
> fits. The CODA_ALLOC() macro calls kzalloc() which means the extra
> memory is just zeroed and it's fine.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c
> index c5234c21b539..910d57e576e2 100644
> --- a/fs/coda/psdev.c
> +++ b/fs/coda/psdev.c
> @@ -124,7 +124,7 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
> hdr.opcode, hdr.unique);
> nbytes = size;
> }
> - CODA_ALLOC(dcbuf, union outputArgs *, nbytes);
> + CODA_ALLOC(dcbuf, union outputArgs *, size);
> if (copy_from_user(dcbuf, buf, nbytes)) {
> CODA_FREE(dcbuf, nbytes);
> retval = -EFAULT;
>
