[PATCH] fs/coda: potential buffer overflow in coda_psdev_write()

[PATCH] fs/coda: potential buffer overflow in coda_psdev_write()

[Dan Carpenter]

"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().

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@oracle.com>

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;

Re: [PATCH] fs/coda: potential buffer overflow in coda_psdev_write()

[Jan Harkes]

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@oracle.com>
> 
> 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;
> 

Re: [PATCH] fs/coda: potential buffer overflow in coda_psdev_write()

[Dan Carpenter]

You misread my commit message, but I have spotted another thing I want
to change so I will resend the patch tomorrow.

On Thu, Jul 12, 2018 at 11:31:00AM -0400, Jan Harkes wrote:
> 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.

It's not *always* the smallest member.  It's *at least* the smallest
member (but not necessarily large enough for the largest).

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

The copy is not the problem, it's coda_downcall().  I did mention that
in my commit message...  Here is how the code looks:

fs/coda/psdev.c
    97  static ssize_t coda_psdev_write(struct file *file, const char __user *buf, 
    98                                  size_t nbytes, loff_t *off)
    99  {
   100          struct venus_comm *vcp = (struct venus_comm *) file->private_data;
   101          struct upc_req *req = NULL;
   102          struct upc_req *tmp;
   103          struct list_head *lh;
   104          struct coda_in_hdr hdr;
   105          ssize_t retval = 0, count = 0;
   106          int error;
   107  
   108          /* Peek at the opcode, uniquefier */
   109          if (copy_from_user(&hdr, buf, 2 * sizeof(u_long)))
   110                  return -EFAULT;
   111  
   112          if (DOWNCALL(hdr.opcode)) {
   113                  union outputArgs *dcbuf;
   114                  int size = sizeof(*dcbuf);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^
size is the whole union.

   115  
   116                  if  ( nbytes < sizeof(struct coda_out_hdr) ) {
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

struct coda_out_hdr is the smallest member.  We are assuming that
nbytes == sizeof(struct coda_out_hdr) so this test is fine.

   117                          pr_warn("coda_downcall opc %d uniq %d, not enough!\n",
   118                                  hdr.opcode, hdr.unique);
   119                          count = nbytes;
   120                          goto out;
   121                  }
   122                  if ( nbytes > size ) {
                             ^^^^^^^^^^^^^
It's not too large.  That's fine.

   123                          pr_warn("downcall opc %d, uniq %d, too much!",
   124                                  hdr.opcode, hdr.unique);
   125                          nbytes = size;
   126                  }
   127                  CODA_ALLOC(dcbuf, union outputArgs *, nbytes);
                                                              ^^^^^^
We allocate enough space for the smallest member.

   128                  if (copy_from_user(dcbuf, buf, nbytes)) {
   129                          CODA_FREE(dcbuf, nbytes);
   130                          retval = -EFAULT;
   131                          goto out;
   132                  }
   133  
   134                  /* what downcall errors does Venus handle ? */
   135                  error = coda_downcall(vcp, hdr.opcode, dcbuf);
                                                               ^^^^^
But coda_downcall() assumes everything is checked properly.  The
references to CodaFid could be out of bounds, for example.
"fid = &out->coda_zapdir.CodaFid;"

   136  
   137                  CODA_FREE(dcbuf, nbytes);
                                         ^^^^^^
Let me update this...  It's not used, but I guess it's uglier to be
wrong as well as unused.  I will resend tomorrow.

regards,
dan carpenter

Re: [PATCH v2] fs/coda: potential buffer overflow in coda_psdev_write()

[Dan Carpenter]

I added the linux-fsdevel list back to the CC.

On Fri, Jul 13, 2018 at 12:16:30PM -0400, Jan Harkes wrote:
> On Fri, Jul 13, 2018 at 06:10:17PM +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 the
> > problem is that we might try to use a larger member.  If "nbytes" is
> > set to sizeof(struct coda_out_hdr) that would cause a problem in
> > coda_downcall() when we try to access &out->coda_zapdir.CodaFid;
> > 
> > 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@oracle.com>
> > ---
> > v2: I forgot to update CODA_FREE() in my first patch.
> 
> You also seem to have missed my comments on your first patch. The
> smallest size test is only to make sure we at least get a complete
> message header.
> 
> If your concern is that a userspace client might send a CODA_ZAPDIR
> downcall but doesn't actually include enough data for the message to
> contain the file identifier to be zapped. I don't think this shouldn't
> be papered over by just passing along some extra zero bytes.
> How would we know these extra zero bytes are fine? How does the
> developer implementing this broken Coda client find out that he is not
> actually sending a properly formatted message and the intended cache
> flush never happens?
> 
> The proper fix should be to check that we received at least enough data
> to fully read the received downcall message based on the opcode in the
> received message header and log/return an error if it doesn't match.
> 

I just wanted to solve the memory corruption without breaking user
space.  What you're proposing sounds more complicated and probably
someone should test it.  Can you fix it and give me a Reported-by tag?

regards,
dan carpenter

Re: [PATCH v2] fs/coda: potential buffer overflow in coda_psdev_write()

[Jan Harkes]

On Fri, Jul 13, 2018 at 10:05:03PM +0300, Dan Carpenter wrote:
> > The proper fix should be to check that we received at least enough data
> > to fully read the received downcall message based on the opcode in the
> > received message header and log/return an error if it doesn't match.
> 
> I just wanted to solve the memory corruption without breaking user
> space.  What you're proposing sounds more complicated and probably
> someone should test it.  Can you fix it and give me a Reported-by tag?

Should not be too hard and I am in the best position to test it, so yes
I will do that.

Jan

[PATCH v3] fs/coda: potential buffer overflow in coda_psdev_write()

[Jan Harkes]

Add checks to make sure the downcall message we got from the Coda cache
manager is large enough to contain the data it is supposed to have.
i.e. when we get a CODA_ZAPDIR we can access &out->coda_zapdir.CodaFid.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com
Signed-off-by: Jan Harkes <jaharkes@cs.cmu.edu>


diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c
index c5234c21b539..43e8cc8dbf26 100644
--- a/fs/coda/psdev.c
+++ b/fs/coda/psdev.c
@@ -105,8 +105,12 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
 	ssize_t retval = 0, count = 0;
 	int error;
 
+        /* make sure there is enough to copy out the (opcode, unique) values */
+        if (nbytes < (2 * sizeof(u_int32_t)))
+                return -EINVAL;
+
         /* Peek at the opcode, uniquefier */
-	if (copy_from_user(&hdr, buf, 2 * sizeof(u_long)))
+	if (copy_from_user(&hdr, buf, 2 * sizeof(u_int32_t)))
 	        return -EFAULT;
 
         if (DOWNCALL(hdr.opcode)) {
@@ -132,7 +136,7 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
 		}
 
 		/* what downcall errors does Venus handle ? */
-		error = coda_downcall(vcp, hdr.opcode, dcbuf);
+		error = coda_downcall(vcp, hdr.opcode, dcbuf, nbytes);
 
 		CODA_FREE(dcbuf, nbytes);
 		if (error) {
diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c
index 1175a1722411..c3c35a80659b 100644
--- a/fs/coda/upcall.c
+++ b/fs/coda/upcall.c
@@ -804,12 +804,42 @@ static int coda_upcall(struct venus_comm *vcp,
  *
  * CODA_REPLACE -- replace one CodaFid with another throughout the name cache */
 
-int coda_downcall(struct venus_comm *vcp, int opcode, union outputArgs *out)
+int coda_downcall(struct venus_comm *vcp, int opcode, union outputArgs *out,
+                  size_t nbytes)
 {
 	struct inode *inode = NULL;
 	struct CodaFid *fid = NULL, *newfid;
 	struct super_block *sb;
 
+        /* make sure we have received enough data from the cache
+         * manager to populate the necessary fields in the buffer */
+	switch (opcode) {
+	case CODA_PURGEUSER:
+                if (nbytes < sizeof(struct coda_purgeuser_out))
+                        return -EINVAL;
+                break;
+
+	case CODA_ZAPDIR:
+                if (nbytes < sizeof(struct coda_zapdir_out))
+                        return -EINVAL;
+		break;
+
+	case CODA_ZAPFILE:
+                if (nbytes < sizeof(struct coda_zapfile_out))
+                        return -EINVAL;
+		break;
+
+	case CODA_PURGEFID:
+                if (nbytes < sizeof(struct coda_purgefid_out))
+                        return -EINVAL;
+		break;
+
+	case CODA_REPLACE:
+                if (nbytes < sizeof(struct coda_replace_out))
+                        return -EINVAL;
+		break;
+	}
+
 	/* Handle invalidation requests. */
 	mutex_lock(&vcp->vc_mutex);
 	sb = vcp->vc_sb;
diff --git a/include/linux/coda_psdev.h b/include/linux/coda_psdev.h
index 15170954aa2b..a553242b30c1 100644
--- a/include/linux/coda_psdev.h
+++ b/include/linux/coda_psdev.h
@@ -60,7 +60,8 @@ int venus_symlink(struct super_block *sb, struct CodaFid *fid,
 int venus_access(struct super_block *sb, struct CodaFid *fid, int mask);
 int venus_pioctl(struct super_block *sb, struct CodaFid *fid,
 		 unsigned int cmd, struct PioctlData *data);
-int coda_downcall(struct venus_comm *vcp, int opcode, union outputArgs *out);
+int coda_downcall(struct venus_comm *vcp, int opcode, union outputArgs *out,
+                  size_t nbytes);
 int venus_fsync(struct super_block *sb, struct CodaFid *fid);
 int venus_statfs(struct dentry *dentry, struct kstatfs *sfs);