From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hurricane.elijah.cs.cmu.edu ([128.2.209.191]:46322 "EHLO hurricane.elijah.cs.cmu.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726775AbeGLQQE (ORCPT ); Thu, 12 Jul 2018 12:16:04 -0400 Date: Thu, 12 Jul 2018 11:31:00 -0400 From: Jan Harkes To: Dan Carpenter Cc: linux-fsdevel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH] fs/coda: potential buffer overflow in coda_psdev_write() Message-ID: <20180712153100.ml2vkv43orrymnax@cs.cmu.edu> References: <20180712123255.rkcszmwdmrloxaki@kili.mountain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180712123255.rkcszmwdmrloxaki@kili.mountain> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > 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; >