From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [patch] vfs: check for integer overflows in posix_acl_alloc() Date: Tue, 25 Jun 2013 07:37:09 +1000 Message-ID: <20130624213709.GT29338@dastard> References: <20130624162719.GB32503@elgon.mountain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alexander Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Dan Carpenter , g@dastard Return-path: Received: from ipmail06.adl6.internode.on.net ([150.101.137.145]:10613 "EHLO ipmail06.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751013Ab3FXVhQ (ORCPT ); Mon, 24 Jun 2013 17:37:16 -0400 Content-Disposition: inline In-Reply-To: <20130624162719.GB32503@elgon.mountain> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Jun 24, 2013 at 07:27:19PM +0300, Dan Carpenter wrote: > We've seen cases where people passed negative numbers to > posix_acl_alloc() and we fixed the caller. For example 093019cf1b "xfs: > fix acl count validation in xfs_acl_from_disk()". Yeah, we failed to detect an on-disk corruption correctly.... > But there are other > places which might be affected like ext4_acl_from_disk() which checks > for negative but doesn't check an upper limit. > > Signed-off-by: Dan Carpenter > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index cea4623..cd7fd2f 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -46,7 +46,12 @@ posix_acl_alloc(int count, gfp_t flags) > { > const size_t size = sizeof(struct posix_acl) + > count * sizeof(struct posix_acl_entry); > - struct posix_acl *acl = kmalloc(size, flags); > + struct posix_acl *acl; > + > + if (count < 0 || count > (SIZE_MAX - sizeof(struct posix_acl) / > + sizeof(struct posix_acl_entry))) > + return NULL; That's not going to solve your problems, because ACLs are limied by filesystem on-disk formats, not SIZE_MAX. The number of valid ACLs is capped by the fact they are stored in xattrs on most (all?) filesystems, and so are limited to fitting into the maximum attribute data length which is 64k. IOWs, the valid ACL limit is per-filesystem, and needs to be checked in the filesystem code as something that is beyond the range of what the on-disk format of the filesystem can handle is a corruption event and needs to be treated as a corruption, not a ENOMEM error.. Cheers, Dave. -- Dave Chinner david@fromorbit.com