| Message ID | 20180712123255.rkcszmwdmrloxaki@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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
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
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
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;
"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>