From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933532AbcLMQHw (ORCPT ); Tue, 13 Dec 2016 11:07:52 -0500 Received: from mx2.suse.de ([195.135.220.15]:36614 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932537AbcLMQHu (ORCPT ); Tue, 13 Dec 2016 11:07:50 -0500 Date: Tue, 13 Dec 2016 17:07:47 +0100 From: Jan Kara To: Mark Salyzyn Cc: Cong Wang , LKML , aneesh.kumar@linux.vnet.ibm.com, Jan Kara Subject: Re: CVE-2016-7097 causes acl leak Message-ID: <20161213160747.GA16622@quack2.suse.cz> References: <3a180415-2f02-c9c0-e1e6-519b5d3115b7@android.com> <2e51e43d-ecfc-a784-df84-0e62b5ad0cc7@android.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2e51e43d-ecfc-a784-df84-0e62b5ad0cc7@android.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 SUSE Labs, CR