CVE-2016-7097 causes acl leak

CVE-2016-7097 causes acl leak

[Mark Salyzyn]

Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences 
of an acl leak.

posix_acl_update_mode(inose, &mode, &acl);

. . .

posix_acl_release(acl);


acl is NULLed in posix_acl_update_mode to signal caller to not update 
the acl; but because it is nulled, it is never released.


Sincerely -- Mark Salyzyn

Re: CVE-2016-7097 causes acl leak

[Greg KH]

On Mon, Dec 05, 2016 at 09:16:31AM -0800, Mark Salyzyn wrote:
> Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences of
> an acl leak.
> 
> posix_acl_update_mode(inose, &mode, &acl);
> 
> . . .
> 
> posix_acl_release(acl);
> 
> 
> acl is NULLed in posix_acl_update_mode to signal caller to not update the
> acl; but because it is nulled, it is never released.

Any reason you didn't cc: the authors of that patch and the correct
mailing list for it (hint, use scripts/get_maintainer.pl on the
patch...)

Try that and see what happens...

thanks,

greg k-h

Re: CVE-2016-7097 causes acl leak

[Cong Wang]

On Mon, Dec 5, 2016 at 9:16 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences of
> an acl leak.
>
> posix_acl_update_mode(inose, &mode, &acl);
>
> . . .
>
> posix_acl_release(acl);
>
>
> acl is NULLed in posix_acl_update_mode to signal caller to not update the
> acl; but because it is nulled, it is never released.

I think you blame the wrong commit, this leak exists before that commit.
Looks like we should just release it before NULL'ing.

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 5955220..edd862a 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -648,8 +648,10 @@ int posix_acl_update_mode(struct inode *inode,
umode_t *mode_p,
        error = posix_acl_equiv_mode(*acl, &mode);
        if (error < 0)
                return error;
-       if (error == 0)
+       if (error == 0) {
+               posix_acl_release(*acl);
                *acl = NULL;
+       }
        if (!in_group_p(inode->i_gid) &&
            !capable_wrt_inode_uidgid(inode, CAP_FSETID))
                mode &= ~S_ISGID;

Re: CVE-2016-7097 causes acl leak

[Jan Kara]

On Sun 11-12-16 16:34:31, Cong Wang wrote:
> On Mon, Dec 5, 2016 at 9:16 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> > Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences of
> > an acl leak.
> >
> > posix_acl_update_mode(inose, &mode, &acl);
> >
> > . . .
> >
> > posix_acl_release(acl);
> >
> >
> > acl is NULLed in posix_acl_update_mode to signal caller to not update the
> > acl; but because it is nulled, it is never released.
> 
> I think you blame the wrong commit, this leak exists before that commit.
> Looks like we should just release it before NULL'ing.

So I agree with you the mentioned commit didn't change anything. I took
care to keep the previous behavior wrt NULLing the acl pointer (obviously I
could have made mistake somewhere but I don't see where). However your
patch is definitely wrong. See e.g. fs/ext2/acl.c: ext2_set_acl() - there we
really want to just clear the pointer. We release the ACL in the caller of
ext2_set_acl().

Mark why do you think we are leaking ACL references and can you be more
specific where exactly it happens?

								Honza

> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 5955220..edd862a 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -648,8 +648,10 @@ int posix_acl_update_mode(struct inode *inode,
> umode_t *mode_p,
>         error = posix_acl_equiv_mode(*acl, &mode);
>         if (error < 0)
>                 return error;
> -       if (error == 0)
> +       if (error == 0) {
> +               posix_acl_release(*acl);
>                 *acl = NULL;
> +       }
>         if (!in_group_p(inode->i_gid) &&
>             !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>                 mode &= ~S_ISGID;
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: CVE-2016-7097 causes acl leak

[Cong Wang]

On Mon, Dec 12, 2016 at 2:46 AM, Jan Kara <jack@suse.cz> wrote:
> So I agree with you the mentioned commit didn't change anything. I took
> care to keep the previous behavior wrt NULLing the acl pointer (obviously I
> could have made mistake somewhere but I don't see where). However your
> patch is definitely wrong. See e.g. fs/ext2/acl.c: ext2_set_acl() - there we
> really want to just clear the pointer. We release the ACL in the caller of
> ext2_set_acl().
>

Hmm, that is very subtle, are you saying we don't carry the change out of
set_posix_acl() because we modify its argument on stack? This is clearly
an anti-pattern in kernel code base.

Re: CVE-2016-7097 causes acl leak

[Mark Salyzyn]

On 12/11/2016 04:34 PM, Cong Wang wrote:
> On Mon, Dec 5, 2016 at 9:16 AM, Mark Salyzyn <salyzyn@android.com> wrote:
>> Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences of
>> an acl leak.
>>
>> posix_acl_update_mode(inose, &mode, &acl);
>>
>> . . .
>>
>> posix_acl_release(acl);
>>
>>
>> acl is NULLed in posix_acl_update_mode to signal caller to not update the
>> acl; but because it is nulled, it is never released.
> I think you blame the wrong commit, this leak exists before that commit.
> Looks like we should just release it before NULL'ing.
>
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 5955220..edd862a 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -648,8 +648,10 @@ int posix_acl_update_mode(struct inode *inode,
> umode_t *mode_p,
>          error = posix_acl_equiv_mode(*acl, &mode);
>          if (error < 0)
>                  return error;
> -       if (error == 0)
> +       if (error == 0) {
> +               posix_acl_release(*acl);
>                  *acl = NULL;
> +       }
>          if (!in_group_p(inode->i_gid) &&
>              !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>                  mode &= ~S_ISGID;

The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.


-- Mark

Re: CVE-2016-7097 causes acl leak

[Cong Wang]

On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@android.com> 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.

Re: CVE-2016-7097 causes acl leak

[Jan Kara]

On Mon 12-12-16 22:26:09, Cong Wang wrote:
> On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@android.com> wrote:
> >
> > The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.
> 
> 
> Only the 9p case is obvious to me:

Agreed and the patch below looks good to me. Please make it a proper patch
(including changelog, sign-off, etc.) and feel free to add my Reviewed-by
tag.

> 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'm not sure what's so unusual about passing a pointer to a local variable
(in fact a function argument but they are no different in C) to another
function. I agree it is not the most straightforward code but it is not that
complicated either...

What is important is that a function that acquires a reference to an acl also
releases that reference. That is a common pattern. I.e. we don't pass "a
reference to an object", we just pass "a pointer to an object" to a
function and guarantee the pointer will stay valid while the function runs.
What does some function (in our case ->set_acl handler) do with the pointer
you passed it is it's internal bussiness.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: CVE-2016-7097 causes acl leak

[Cong Wang]

On Tue, Dec 13, 2016 at 3:28 AM, Jan Kara <jack@suse.cz> wrote:
> On Mon 12-12-16 22:26:09, Cong Wang wrote:
>> On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>> >
>> > The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.
>>
>>
>> Only the 9p case is obvious to me:
>
> Agreed and the patch below looks good to me. Please make it a proper patch
> (including changelog, sign-off, etc.) and feel free to add my Reviewed-by
> tag.

Done.


>> The rest are anti-pattern (modifying parameters on stack via address)
>> but look correct.
>
> I'm not sure what's so unusual about passing a pointer to a local variable
> (in fact a function argument but they are no different in C) to another
> function. I agree it is not the most straightforward code but it is not that
> complicated either...

Function arguments technically belong to caller's context, while local
variables belong to callee's. I have never seen such a use case in kernel
code base, this is why I think it is anti-pattern.

>
> What is important is that a function that acquires a reference to an acl also
> releases that reference. That is a common pattern. I.e. we don't pass "a
> reference to an object", we just pass "a pointer to an object" to a
> function and guarantee the pointer will stay valid while the function runs.
> What does some function (in our case ->set_acl handler) do with the pointer
> you passed it is it's internal bussiness.
>

But passing a pointer to a pointer usually indicates we need to save
the pointer,
in this case we actually don't, it is discarded as soon as the caller returns.

Thanks.

Re: CVE-2016-7097 causes acl leak

[Mark Salyzyn]

On 12/12/2016 10:26 PM, Cong Wang wrote:
> On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@android.com> 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);

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@android.com> 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@suse.com>
SUSE Labs, CR

Re: CVE-2016-7097 causes acl leak

[Mark Salyzyn]

On 12/12/2016 10:26 PM, Cong Wang wrote:
> On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@android.com> 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.

Greg KH: Beware that this similar fix needs to be applied to _backports_ 
to stable kernel trees on other filesystem driver that have the same 
pattern (with local posix_acl_release(acl) calls). I have found that 
depending on vintage these would include this driver 9p, and possibly 
gfs2, jfs and xfs. Be aware.


Sincerely -- Mark Salyzyn

Re: CVE-2016-7097 causes acl leak

[Greg KH]

On Tue, Dec 13, 2016 at 03:42:58PM -0800, 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@android.com> 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.
> 
> Greg KH: Beware that this similar fix needs to be applied to _backports_ to
> stable kernel trees on other filesystem driver that have the same pattern
> (with local posix_acl_release(acl) calls). I have found that depending on
> vintage these would include this driver 9p, and possibly gfs2, jfs and xfs.
> Be aware.

I don't understand what you mean here.  What needs to be "backported" to
the stable tree?  What commit in Linus's tree do I pick?  If not a
commit there, where is it?

totally confused,

greg k-h

Re: CVE-2016-7097 causes acl leak

[Mark Salyzyn]

On 12/13/2016 04:00 PM, Greg KH wrote:
> On Tue, Dec 13, 2016 at 03:42:58PM -0800, 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@android.com> 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.
>> Greg KH: Beware that this similar fix needs to be applied to _backports_ to
>> stable kernel trees on other filesystem driver that have the same pattern
>> (with local posix_acl_release(acl) calls). I have found that depending on
>> vintage these would include this driver 9p, and possibly gfs2, jfs and xfs.
>> Be aware.
> I don't understand what you mean here.  What needs to be "backported" to
> the stable tree?  What commit in Linus's tree do I pick?  If not a
> commit there, where is it?
>
> totally confused,
>
> greg k-h

In 3.10-stable if you took the original CVE-2016-7097 fix it could break 
four file system drivers, the fix for each would 'look like' this one 
fix for the 9p driver.

-- Mark

Re: CVE-2016-7097 causes acl leak

[Greg KH]

On Wed, Dec 14, 2016 at 12:20:50PM -0800, Mark Salyzyn wrote:
> On 12/13/2016 04:00 PM, Greg KH wrote:
> > On Tue, Dec 13, 2016 at 03:42:58PM -0800, 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@android.com> 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.
> > > Greg KH: Beware that this similar fix needs to be applied to _backports_ to
> > > stable kernel trees on other filesystem driver that have the same pattern
> > > (with local posix_acl_release(acl) calls). I have found that depending on
> > > vintage these would include this driver 9p, and possibly gfs2, jfs and xfs.
> > > Be aware.
> > I don't understand what you mean here.  What needs to be "backported" to
> > the stable tree?  What commit in Linus's tree do I pick?  If not a
> > commit there, where is it?
> > 
> > totally confused,
> > 
> > greg k-h
> 
> In 3.10-stable if you took the original CVE-2016-7097 fix it could break
> four file system drivers, the fix for each would 'look like' this one fix
> for the 9p driver.

Did I take the fix in 3.10-stable?  What was the git commit id?  Is 3.10
"broken" in this way?  Is any other stable kernel broken?

I still don't have any idea of what is going on here...

greg k-h

Re: CVE-2016-7097 causes acl leak

[Mark Salyzyn]

On 12/14/2016 03:30 PM, Greg KH wrote:
> On Wed, Dec 14, 2016 at 12:20:50PM -0800, Mark Salyzyn wrote:
>> On 12/13/2016 04:00 PM, Greg KH wrote:
>>> On Tue, Dec 13, 2016 at 03:42:58PM -0800, 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@android.com> 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.
>>>> Greg KH: Beware that this similar fix needs to be applied to _backports_ to
>>>> stable kernel trees on other filesystem driver that have the same pattern
>>>> (with local posix_acl_release(acl) calls). I have found that depending on
>>>> vintage these would include this driver 9p, and possibly gfs2, jfs and xfs.
>>>> Be aware.
>>> I don't understand what you mean here.  What needs to be "backported" to
>>> the stable tree?  What commit in Linus's tree do I pick?  If not a
>>> commit there, where is it?
>>>
>>> totally confused,
>>>
>>> greg k-h
>> In 3.10-stable if you took the original CVE-2016-7097 fix it could break
>> four file system drivers, the fix for each would 'look like' this one fix
>> for the 9p driver.
> Did I take the fix in 3.10-stable?  What was the git commit id?  Is 3.10
> "broken" in this way?  Is any other stable kernel broken?
>
> I still don't have any idea of what is going on here...
>
> greg k-h

Nothing is going on here, it is a heads up, eventually CVE's get 
backported to stable as we do take them in through those paths. Telling 
you to be aware that the original commit causes a leak, and my 
experience has found that the leak affects these four file system drivers.


-- Mark

Re: CVE-2016-7097 causes acl leak

[Jan Kara]

On Thu 15-12-16 07:22:30, Mark Salyzyn wrote:
> On 12/14/2016 03:30 PM, Greg KH wrote:
> >On Wed, Dec 14, 2016 at 12:20:50PM -0800, Mark Salyzyn wrote:
> >>On 12/13/2016 04:00 PM, Greg KH wrote:
> >>>On Tue, Dec 13, 2016 at 03:42:58PM -0800, 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@android.com> 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.
> >>>>Greg KH: Beware that this similar fix needs to be applied to _backports_ to
> >>>>stable kernel trees on other filesystem driver that have the same pattern
> >>>>(with local posix_acl_release(acl) calls). I have found that depending on
> >>>>vintage these would include this driver 9p, and possibly gfs2, jfs and xfs.
> >>>>Be aware.
> >>>I don't understand what you mean here.  What needs to be "backported" to
> >>>the stable tree?  What commit in Linus's tree do I pick?  If not a
> >>>commit there, where is it?
> >>>
> >>>totally confused,
> >>>
> >>>greg k-h
> >>In 3.10-stable if you took the original CVE-2016-7097 fix it could break
> >>four file system drivers, the fix for each would 'look like' this one fix
> >>for the 9p driver.
> >Did I take the fix in 3.10-stable?  What was the git commit id?  Is 3.10
> >"broken" in this way?  Is any other stable kernel broken?
> >
> >I still don't have any idea of what is going on here...
> >
> >greg k-h
> 
> Nothing is going on here, it is a heads up, eventually CVE's get backported
> to stable as we do take them in through those paths. Telling you to be aware
> that the original commit causes a leak, and my experience has found that the
> leak affects these four file system drivers.

Original commit (073931017b49) fixing the CVE does not contain the leak.
The leak in 9p was there before that commit. But yes, a naive backport of
that commit into 3.10 will introduce new similar leaks into xfs and others.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: CVE-2016-7097 causes acl leak

[Jan Kara]

On Mon 12-12-16 16:26:00, Mark Salyzyn wrote:
> On 12/11/2016 04:34 PM, Cong Wang wrote:
> >On Mon, Dec 5, 2016 at 9:16 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> >>Commit 073931017b49d9458aa351605b43a7e34598caef has several occurrences of
> >>an acl leak.
> >>
> >>posix_acl_update_mode(inose, &mode, &acl);
> >>
> >>. . .
> >>
> >>posix_acl_release(acl);
> >>
> >>
> >>acl is NULLed in posix_acl_update_mode to signal caller to not update the
> >>acl; but because it is nulled, it is never released.
> >I think you blame the wrong commit, this leak exists before that commit.
> >Looks like we should just release it before NULL'ing.
> >
> >diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> >index 5955220..edd862a 100644
> >--- a/fs/posix_acl.c
> >+++ b/fs/posix_acl.c
> >@@ -648,8 +648,10 @@ int posix_acl_update_mode(struct inode *inode,
> >umode_t *mode_p,
> >         error = posix_acl_equiv_mode(*acl, &mode);
> >         if (error < 0)
> >                 return error;
> >-       if (error == 0)
> >+       if (error == 0) {
> >+               posix_acl_release(*acl);
> >                 *acl = NULL;
> >+       }
> >         if (!in_group_p(inode->i_gid) &&
> >             !capable_wrt_inode_uidgid(inode, CAP_FSETID))
> >                 mode &= ~S_ISGID;
> 
> The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.

So I agree 9p leaks acl reference (and it was buggy even before my commit).
GFS2, JFS, and XFS seem to be OK as far as I can tell.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR