linux-kernel - Re: [patch] vfs: check for integer overflows in posix_acl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130624213709.GT29338@dastard>
Date:	Tue, 25 Jun 2013 07:37:09 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>, g@...tard
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch] vfs: check for integer overflows in posix_acl_alloc()

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 <dan.carpenter@...cle.com>
> 
> 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@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/