archive mirror
 help / color / mirror / Atom feed
From: Jan Harkes <>
To: Dan Carpenter <>
Subject: Re: [PATCH] fs/coda: potential buffer overflow in coda_psdev_write()
Date: Thu, 12 Jul 2018 11:31:00 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20180712123255.rkcszmwdmrloxaki@kili.mountain>

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

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

Even if we allocate size instead of nbytes, we still wouldn't
copy_from_user more than nbytes anyway.


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

  reply	other threads:[~2018-07-12 16:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-12 12:32 [PATCH] fs/coda: potential buffer overflow in coda_psdev_write() Dan Carpenter
2018-07-12 15:31 ` Jan Harkes [this message]
2018-07-12 15:50   ` Dan Carpenter
     [not found] ` <20180713151017.lxbv4eljvd6olziq@kili.mountain>
     [not found]   ` <>
2018-07-13 19:05     ` [PATCH v2] " Dan Carpenter
2018-07-13 19:08       ` Jan Harkes
2018-07-14  2:24       ` [PATCH v3] " Jan Harkes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).