Re: CVE-2016-7097 causes acl leak

[Jan Kara]

On Tue 13-12-16 07:55:23, Mark Salyzyn wrote:
> On 12/12/2016 10:26 PM, Cong Wang wrote:
> >On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
> >>The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.
> >
> >Only the 9p case is obvious to me:
> >
> >diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> >index b3c2cc7..082d227 100644
> >--- a/fs/9p/acl.c
> >+++ b/fs/9p/acl.c
> >@@ -277,6 +277,7 @@ static int v9fs_xattr_set_acl(const struct
> >xattr_handler *handler,
> >         case ACL_TYPE_ACCESS:
> >                 if (acl) {
> >                         struct iattr iattr;
> >+                       struct posix_acl *old_acl = acl;
> >
> >                         retval = posix_acl_update_mode(inode,
> >&iattr.ia_mode, &acl);
> >                         if (retval)
> >@@ -287,6 +288,7 @@ static int v9fs_xattr_set_acl(const struct
> >xattr_handler *handler,
> >                                  * by the mode bits. So don't
> >                                  * update ACL.
> >                                  */
> >+                               posix_acl_release(old_acl);
> >                                 value = NULL;
> >                                 size = 0;
> >                         }
> >
> >
> >The rest are anti-pattern (modifying parameters on stack via address)
> >but look correct
> I chose to modify posix_acl_update_mode as follows and set release_acl in
> the specific drivers, it clears the *acl reference to nul preventing
> posix_acl_release from functioning in these other driver paths:
> 
>  */
> int posix_acl_update_mode(struct inode *inode, umode_t *mode_p,
>               struct posix_acl **acl, ++++bool release_acl)
> {
>     umode_t mode = inode->i_mode;
>     int error;
> 
>     error = posix_acl_equiv_mode(*acl, &mode);
>     if (error < 0)
>         return error;
>     if (error == 0) +{
> +        if (release_acl)
> +            posix_acl_release(*acl);
>         *acl = NULL;
> +    }
>     if (!in_group_p(inode->i_gid) &&
>         !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>         mode &= ~S_ISGID;
>     *mode_p = mode;
>     return 0;
> }
> EXPORT_SYMBOL(posix_acl_update_mode);

I much prefer what Cong Wang did. These bool parameters which subtly change
the function behavior are too easy to screw up and it is not really
visible by just looking at the function call...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR