From: Dan Carpenter <dan.carpenter@oracle.com>
To: linux-fsdevel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] fs/coda: potential buffer overflow in coda_psdev_write()
Date: Thu, 12 Jul 2018 18:50:35 +0300 [thread overview]
Message-ID: <20180712155034.7ihdmp3mnzlkc6hb@mwanda> (raw)
In-Reply-To: <20180712153100.ml2vkv43orrymnax@cs.cmu.edu>
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
next prev parent reply other threads:[~2018-07-12 16:00 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
2018-07-12 15:50 ` Dan Carpenter [this message]
[not found] ` <20180713151017.lxbv4eljvd6olziq@kili.mountain>
[not found] ` <20180713161630.olrwa2n2tnpqbmlt@cs.cmu.edu>
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180712155034.7ihdmp3mnzlkc6hb@mwanda \
--to=dan.carpenter@oracle.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).