[PATCH] cgroups: fix possible use after free

[PATCH] cgroups: fix possible use after free

[Li Zefan]

In cgroup_kill_sb(), root is freed before sb is detached from the list,
so another sget() may find this sb and call cgroup_test_super(),
which will access the root that has been freed.

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5a54ff4..42fd3f2 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 
 	mutex_unlock(&cgroup_mutex);
 
-	kfree(root);
 	kill_litter_super(sb);
+	kfree(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
-- 
1.5.4.rc3

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> In cgroup_kill_sb(), root is freed before sb is detached from the list,
> so another sget() may find this sb and call cgroup_test_super(),
> which will access the root that has been freed.

I think that I'd assumed that by the time we get to cgroup_kill_sb()
there's no chance of the sb being resurrected by sget().

If anyone can call cgroup_test_super() on the sb that we're trying to
free, isn't that bad? Since even with this patch we free root just
after calling kill_litter_sb(), if anyone tries to reuse that super
block then they're toast.

Would it be better to set sb->s_fs_info to NULL, before calling
kill_litter_sb(), and have cgroup_test_super() just return 0 for any
sb with a NULL sb->s_fs_info.

Paul

>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 5a54ff4..42fd3f2 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
>
>        mutex_unlock(&cgroup_mutex);
>
> -       kfree(root);
>        kill_litter_super(sb);
> +       kfree(root);
>  }
>
>  static struct file_system_type cgroup_fs_type = {
> --
> 1.5.4.rc3
>

Re: [PATCH] cgroups: fix possible use after free

[Al Viro]

On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
> > so another sget() may find this sb and call cgroup_test_super(),
> > which will access the root that has been freed.
> 
> I think that I'd assumed that by the time we get to cgroup_kill_sb()
> there's no chance of the sb being resurrected by sget().

There is none.  grab_super() will fail to get it, so sget() will go
through retry logics.  Which doesn't mean that test won't be called
on it in the meanwhile.

Re: [PATCH] cgroups: fix possible use after free

[Al Viro]

On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
> > so another sget() may find this sb and call cgroup_test_super(),
> > which will access the root that has been freed.
> 
> I think that I'd assumed that by the time we get to cgroup_kill_sb()
> there's no chance of the sb being resurrected by sget().

There is none.  grab_super() will fail to get it, so sget() will go
through retry logics.  Which doesn't mean that test won't be called
on it in the meanwhile.

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 4:45 AM, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:
> On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
>> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
>> > so another sget() may find this sb and call cgroup_test_super(),
>> > which will access the root that has been freed.
>>
>> I think that I'd assumed that by the time we get to cgroup_kill_sb()
>> there's no chance of the sb being resurrected by sget().
>
> There is none.  grab_super() will fail to get it, so sget() will go
> through retry logics.  Which doesn't mean that test won't be called
> on it in the meanwhile.

OK, so Zefan's patch looks like the safest way to fix this particular
issue. I think I see some other potential races with
cgroup_test_super() though - we probably need to synchronize against
the changing of a root's subsys_bits in rebind_subsystems(). Taking
cgroup_mutex around the call to sget() would certainly provide that,
but I'd have to check whether it causes locking cycles.

Paul

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 4:45 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
>> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
>> > so another sget() may find this sb and call cgroup_test_super(),
>> > which will access the root that has been freed.
>>
>> I think that I'd assumed that by the time we get to cgroup_kill_sb()
>> there's no chance of the sb being resurrected by sget().
>
> There is none.  grab_super() will fail to get it, so sget() will go
> through retry logics.  Which doesn't mean that test won't be called
> on it in the meanwhile.

OK, so Zefan's patch looks like the safest way to fix this particular
issue. I think I see some other potential races with
cgroup_test_super() though - we probably need to synchronize against
the changing of a root's subsys_bits in rebind_subsystems(). Taking
cgroup_mutex around the call to sget() would certainly provide that,
but I'd have to check whether it causes locking cycles.

Paul

Re: [PATCH] cgroups: fix possible use after free

[Al Viro]

On Tue, Feb 10, 2009 at 04:01:07PM -0800, Paul Menage wrote:
> On Tue, Feb 10, 2009 at 4:45 AM, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:
> > On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
> >> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> >> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
> >> > so another sget() may find this sb and call cgroup_test_super(),
> >> > which will access the root that has been freed.
> >>
> >> I think that I'd assumed that by the time we get to cgroup_kill_sb()
> >> there's no chance of the sb being resurrected by sget().
> >
> > There is none.  grab_super() will fail to get it, so sget() will go
> > through retry logics.  Which doesn't mean that test won't be called
> > on it in the meanwhile.
> 
> OK, so Zefan's patch looks like the safest way to fix this particular
> issue. I think I see some other potential races with
> cgroup_test_super() though - we probably need to synchronize against
> the changing of a root's subsys_bits in rebind_subsystems(). Taking
> cgroup_mutex around the call to sget() would certainly provide that,
> but I'd have to check whether it causes locking cycles.

Deadlock.  The whole point of what sget() is doing with retries is this:
if you have a matching fs in the middle of shutdown, you obviously can't
pick its sb (it's halfway shut down) and you can't start bringing your
instance up until the halfdead one is gone.

So you can't wrap sget() into a mutex that might be grabbed at some
point of ->kill_sb() or it'll wait forever in that scenario.  Note
that test is essentially atomic; we are doing it fast for all superblocks
of this type.

If we find a match, we go ahead and pick that superblock.  So there's no
problem just assuming we'd won the race, got existing superblock and
change (if any) of ->subsys_bits simply happened just after that.

AFAICS, that's perfectly sane - you are talking about the race between
mount picking fs instance with the same options and remount changing
said instance.  Other callers of rebind_subsystems() do not apply -
one in your ->get_sb() won't change the field anyway and one in
->kill_sb() is irrelevant since the superblock will be rejected by
grab_super().

So no exclusion is needed there at all.  If you don't want later remount
of the first mount to affect these flags of second one you shouldn't share
the superblock at all, obviously...

Re: [PATCH] cgroups: fix possible use after free

[Al Viro]

On Tue, Feb 10, 2009 at 04:01:07PM -0800, Paul Menage wrote:
> On Tue, Feb 10, 2009 at 4:45 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Tue, Feb 10, 2009 at 02:15:36AM -0800, Paul Menage wrote:
> >> On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> >> > In cgroup_kill_sb(), root is freed before sb is detached from the list,
> >> > so another sget() may find this sb and call cgroup_test_super(),
> >> > which will access the root that has been freed.
> >>
> >> I think that I'd assumed that by the time we get to cgroup_kill_sb()
> >> there's no chance of the sb being resurrected by sget().
> >
> > There is none.  grab_super() will fail to get it, so sget() will go
> > through retry logics.  Which doesn't mean that test won't be called
> > on it in the meanwhile.
> 
> OK, so Zefan's patch looks like the safest way to fix this particular
> issue. I think I see some other potential races with
> cgroup_test_super() though - we probably need to synchronize against
> the changing of a root's subsys_bits in rebind_subsystems(). Taking
> cgroup_mutex around the call to sget() would certainly provide that,
> but I'd have to check whether it causes locking cycles.

Deadlock.  The whole point of what sget() is doing with retries is this:
if you have a matching fs in the middle of shutdown, you obviously can't
pick its sb (it's halfway shut down) and you can't start bringing your
instance up until the halfdead one is gone.

So you can't wrap sget() into a mutex that might be grabbed at some
point of ->kill_sb() or it'll wait forever in that scenario.  Note
that test is essentially atomic; we are doing it fast for all superblocks
of this type.

If we find a match, we go ahead and pick that superblock.  So there's no
problem just assuming we'd won the race, got existing superblock and
change (if any) of ->subsys_bits simply happened just after that.

AFAICS, that's perfectly sane - you are talking about the race between
mount picking fs instance with the same options and remount changing
said instance.  Other callers of rebind_subsystems() do not apply -
one in your ->get_sb() won't change the field anyway and one in
->kill_sb() is irrelevant since the superblock will be rejected by
grab_super().

So no exclusion is needed there at all.  If you don't want later remount
of the first mount to affect these flags of second one you shouldn't share
the superblock at all, obviously...

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 5:19 PM, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:
>
> So you can't wrap sget() into a mutex that might be grabbed at some
> point of ->kill_sb() or it'll wait forever in that scenario.

Good point.

>
> So no exclusion is needed there at all.  If you don't want later remount
> of the first mount to affect these flags of second one you shouldn't share
> the superblock at all, obviously...

I have plans for some slightly more complex state in that structure -
I'll probably just add a spinlock around accesses to those fields so
that we can avoid exposing any inconsistent state.

Paul

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 5:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> So you can't wrap sget() into a mutex that might be grabbed at some
> point of ->kill_sb() or it'll wait forever in that scenario.

Good point.

>
> So no exclusion is needed there at all.  If you don't want later remount
> of the first mount to affect these flags of second one you shouldn't share
> the superblock at all, obviously...

I have plans for some slightly more complex state in that structure -
I'll probably just add a spinlock around accesses to those fields so
that we can avoid exposing any inconsistent state.

Paul

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> In cgroup_kill_sb(), root is freed before sb is detached from the list,
> so another sget() may find this sb and call cgroup_test_super(),
> which will access the root that has been freed.

I think that I'd assumed that by the time we get to cgroup_kill_sb()
there's no chance of the sb being resurrected by sget().

If anyone can call cgroup_test_super() on the sb that we're trying to
free, isn't that bad? Since even with this patch we free root just
after calling kill_litter_sb(), if anyone tries to reuse that super
block then they're toast.

Would it be better to set sb->s_fs_info to NULL, before calling
kill_litter_sb(), and have cgroup_test_super() just return 0 for any
sb with a NULL sb->s_fs_info.

Paul

>
> Reported-by: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> ---
>  kernel/cgroup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 5a54ff4..42fd3f2 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
>
>        mutex_unlock(&cgroup_mutex);
>
> -       kfree(root);
>        kill_litter_super(sb);
> +       kfree(root);
>  }
>
>  static struct file_system_type cgroup_fs_type = {
> --
> 1.5.4.rc3
>

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> In cgroup_kill_sb(), root is freed before sb is detached from the list,
> so another sget() may find this sb and call cgroup_test_super(),
> which will access the root that has been freed.
>
> Reported-by: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Acked-by: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Seems like the safest fix.

Paul

Re: [PATCH] cgroups: fix possible use after free

[Paul Menage]

On Tue, Feb 10, 2009 at 1:31 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> In cgroup_kill_sb(), root is freed before sb is detached from the list,
> so another sget() may find this sb and call cgroup_test_super(),
> which will access the root that has been freed.
>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

Acked-by: Paul Menage <menage@google.com>

Seems like the safest fix.

Paul

[PATCH] cgroups: fix possible use after free

[Li Zefan]

In cgroup_kill_sb(), root is freed before sb is detached from the list,
so another sget() may find this sb and call cgroup_test_super(),
which will access the root that has been freed.

Reported-by: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5a54ff4..42fd3f2 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 
 	mutex_unlock(&cgroup_mutex);
 
-	kfree(root);
 	kill_litter_super(sb);
+	kfree(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
-- 
1.5.4.rc3