[patch] vfs: check for integer overflows in posix_acl_alloc()

[patch] vfs: check for integer overflows in posix_acl_alloc()

[Dan Carpenter]

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()".  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@oracle.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;
+	acl = kmalloc(size, flags);
 	if (acl)
 		posix_acl_init(acl, count);
 	return acl;

Re: [patch] vfs: check for integer overflows in posix_acl_alloc()

[Dan Carpenter]

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()".  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@oracle.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)))

Gar.  I completely screwed that up.  Please ignore this.  I will
send a better patch in a couple days.  I am sorry.

regards,
dan carpenter

> +		return NULL;
> +	acl = kmalloc(size, flags);
>  	if (acl)
>  		posix_acl_init(acl, count);
>  	return acl;

Re: [patch] vfs: check for integer overflows in posix_acl_alloc()

[Dave Chinner]

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@oracle.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@fromorbit.com

Re: [patch] vfs: check for integer overflows in posix_acl_alloc()

[Dan Carpenter]

Yeah.  You're right.  When I look at it more closely the ext4
example is capped ext4_get_acl().  Sorry for the noise.

regards,
dan carpenter