xfs
[Top] [All Lists]

Re: [PATCH] repair: validate acl count before reading it

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] repair: validate acl count before reading it
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 16 Nov 2011 11:23:23 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111115080714.GA24931@xxxxxxxxxxxxx>
References: <20111115080714.GA24931@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Nov 15, 2011 at 03:07:15AM -0500, Christoph Hellwig wrote:
> This prevents a segfault on a filesystem so badly corrupted by the RAID
> controller that it could be considered fuzzed.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfsprogs-dev/repair/attr_repair.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/attr_repair.c    2011-11-14 20:03:27.000000000 
> +0000
> +++ xfsprogs-dev/repair/attr_repair.c 2011-11-14 20:20:55.000000000 +0000
> @@ -931,8 +931,8 @@ process_longform_attr(
>  }
>  
>  
> -static xfs_acl_t *
> -xfs_acl_from_disk(xfs_acl_disk_t *dacl)
> +static int
> +xfs_acl_from_disk(struct xfs_acl **aclp, struct xfs_acl_disk *dacl)
>  {
>       int                     count;
>       xfs_acl_t               *acl;
> @@ -940,10 +940,22 @@ xfs_acl_from_disk(xfs_acl_disk_t *dacl)
>       xfs_acl_entry_disk_t    *dace, *end;
>  
>       count = be32_to_cpu(dacl->acl_cnt);
> +     if (count > XFS_ACL_MAX_ENTRIES) {
> +             do_warn(_("to larget ACL, size %d"), count);

                           "Too many ACL entries, count %d\n" 

> +             *aclp = NULL;
> +             return EINVAL;
> +     }
> +
> +
>       end = &dacl->acl_entry[0] + count;
>       acl = malloc((int)((char *)end - (char *)dacl));
> -     if (!acl)
> -             return NULL;
> +     if (!acl) {
> +             do_warn(_("cannot malloc enough for ACL attribute\n"));
> +             do_warn(_("SKIPPING this ACL\n"));

Should you put that same "Skipping" message for all the error cases?

FWIW, should that status be stored somewhere so that when repair
completes it can emit a warning saying something like:

WARNING: ACLs were not correctly validated. You need to ensure ACLs are
        consistently and appropriately applied to your filesytem.

Regardless, that can be done as a separate patch.

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>