[PATCH] fsnotify: don't put user context if it was never assigned

[PATCH] fsnotify: don't put user context if it was never assigned

[Sasha Levin]

On some failure paths we may attempt to free user context even
if it wasn't assigned yet. This will cause a NULL ptr deref
and a kernel BUG.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 fs/notify/inotify/inotify_fsnotify.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 43ab1e1..9c8187e 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
 	/* ideally the idr is empty and we won't hit the BUG in the callback */
 	idr_for_each(&group->inotify_data.idr, idr_callback, group);
 	idr_destroy(&group->inotify_data.idr);
-	atomic_dec(&group->inotify_data.user->inotify_devs);
-	free_uid(group->inotify_data.user);
+	if (group->inotify_data.user) {
+		atomic_dec(&group->inotify_data.user->inotify_devs);
+		free_uid(group->inotify_data.user);
+	}
 }
 
 static void inotify_free_event(struct fsnotify_event *fsn_event)
-- 
1.7.10.4

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Sasha Levin]

Ping? This is a NULL ptr deref that userspace can trigger.

On 07/29/2014 09:25 AM, Sasha Levin wrote:
> On some failure paths we may attempt to free user context even
> if it wasn't assigned yet. This will cause a NULL ptr deref
> and a kernel BUG.
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  fs/notify/inotify/inotify_fsnotify.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 43ab1e1..9c8187e 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>  	/* ideally the idr is empty and we won't hit the BUG in the callback */
>  	idr_for_each(&group->inotify_data.idr, idr_callback, group);
>  	idr_destroy(&group->inotify_data.idr);
> -	atomic_dec(&group->inotify_data.user->inotify_devs);
> -	free_uid(group->inotify_data.user);
> +	if (group->inotify_data.user) {
> +		atomic_dec(&group->inotify_data.user->inotify_devs);
> +		free_uid(group->inotify_data.user);
> +	}
>  }
>  
>  static void inotify_free_event(struct fsnotify_event *fsn_event)
> 

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Sasha Levin]

Ping?

On 09/03/2014 05:31 PM, Sasha Levin wrote:
> Ping? This is a NULL ptr deref that userspace can trigger.
> 
> On 07/29/2014 09:25 AM, Sasha Levin wrote:
>> On some failure paths we may attempt to free user context even
>> if it wasn't assigned yet. This will cause a NULL ptr deref
>> and a kernel BUG.
>>
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> ---
>>  fs/notify/inotify/inotify_fsnotify.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
>> index 43ab1e1..9c8187e 100644
>> --- a/fs/notify/inotify/inotify_fsnotify.c
>> +++ b/fs/notify/inotify/inotify_fsnotify.c
>> @@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>>  	/* ideally the idr is empty and we won't hit the BUG in the callback */
>>  	idr_for_each(&group->inotify_data.idr, idr_callback, group);
>>  	idr_destroy(&group->inotify_data.idr);
>> -	atomic_dec(&group->inotify_data.user->inotify_devs);
>> -	free_uid(group->inotify_data.user);
>> +	if (group->inotify_data.user) {
>> +		atomic_dec(&group->inotify_data.user->inotify_devs);
>> +		free_uid(group->inotify_data.user);
>> +	}
>>  }
>>  
>>  static void inotify_free_event(struct fsnotify_event *fsn_event)
>>
> 

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Heinrich Schuchardt]

Hello Sasha,

I have CCed Jan, because he has been the only one working on this file 
in the last 18 months.

A failure path in which group->inotify_data.user is not yet assigned 
starts here:

static struct fsnotify_group *inotify_new_group(unsigned int max_events)
{
...
	oevent = kmalloc(sizeof(struct inotify_event_info), GFP_KERNEL);
	if (unlikely(!oevent)) {
		fsnotify_destroy_group(group);
		return ERR_PTR(-ENOMEM);
	}

So your check
   if (group->inotify_data.user)
is reasonable.

3.17.0-rc3 compiles with the patch applied.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

@Andrew
Please, add the patch to the mm-tree.

Best regards

Heinrich Schuchardt

On 29.07.2014 15:25, Sasha Levin wrote:
> On some failure paths we may attempt to free user context even
> if it wasn't assigned yet. This will cause a NULL ptr deref
> and a kernel BUG.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>   fs/notify/inotify/inotify_fsnotify.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 43ab1e1..9c8187e 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>   	/* ideally the idr is empty and we won't hit the BUG in the callback */
>   	idr_for_each(&group->inotify_data.idr, idr_callback, group);
>   	idr_destroy(&group->inotify_data.idr);
> -	atomic_dec(&group->inotify_data.user->inotify_devs);
> -	free_uid(group->inotify_data.user);
> +	if (group->inotify_data.user) {
> +		atomic_dec(&group->inotify_data.user->inotify_devs);
> +		free_uid(group->inotify_data.user);
> +	}
>   }
>
>   static void inotify_free_event(struct fsnotify_event *fsn_event)
>

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Jan Kara]

On Thu 11-09-14 19:52:09, Heinrich Schuchardt wrote:
> Hello Sasha,
> 
> I have CCed Jan, because he has been the only one working on this
> file in the last 18 months.
> 
> A failure path in which group->inotify_data.user is not yet assigned
> starts here:
> 
> static struct fsnotify_group *inotify_new_group(unsigned int max_events)
> {
> ...
> 	oevent = kmalloc(sizeof(struct inotify_event_info), GFP_KERNEL);
> 	if (unlikely(!oevent)) {
> 		fsnotify_destroy_group(group);
> 		return ERR_PTR(-ENOMEM);
> 	}
> 
> So your check
>   if (group->inotify_data.user)
> is reasonable.
> 
> 3.17.0-rc3 compiles with the patch applied.
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> @Andrew
> Please, add the patch to the mm-tree.
  Yeah, the patch looks good. Thanks for CC. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> On 29.07.2014 15:25, Sasha Levin wrote:
> >On some failure paths we may attempt to free user context even
> >if it wasn't assigned yet. This will cause a NULL ptr deref
> >and a kernel BUG.
> >
> >Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >---
> >  fs/notify/inotify/inotify_fsnotify.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> >index 43ab1e1..9c8187e 100644
> >--- a/fs/notify/inotify/inotify_fsnotify.c
> >+++ b/fs/notify/inotify/inotify_fsnotify.c
> >@@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
> >  	/* ideally the idr is empty and we won't hit the BUG in the callback */
> >  	idr_for_each(&group->inotify_data.idr, idr_callback, group);
> >  	idr_destroy(&group->inotify_data.idr);
> >-	atomic_dec(&group->inotify_data.user->inotify_devs);
> >-	free_uid(group->inotify_data.user);
> >+	if (group->inotify_data.user) {
> >+		atomic_dec(&group->inotify_data.user->inotify_devs);
> >+		free_uid(group->inotify_data.user);
> >+	}
> >  }
> >
> >  static void inotify_free_event(struct fsnotify_event *fsn_event)
> >
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Andrew Morton]

On Tue, 29 Jul 2014 09:25:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:

> On some failure paths we may attempt to free user context even
> if it wasn't assigned yet. This will cause a NULL ptr deref
> and a kernel BUG.

Are you able to identify "some failure paths"?  I spent some time
grepping, but it's a pain.

Please try to include such info in changelogs because reviewers (ie,
me) might want to review those callers to decide whether the bug lies
elsewhere.

> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -165,8 +165,10 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>  	/* ideally the idr is empty and we won't hit the BUG in the callback */
>  	idr_for_each(&group->inotify_data.idr, idr_callback, group);
>  	idr_destroy(&group->inotify_data.idr);
> -	atomic_dec(&group->inotify_data.user->inotify_devs);
> -	free_uid(group->inotify_data.user);
> +	if (group->inotify_data.user) {
> +		atomic_dec(&group->inotify_data.user->inotify_devs);
> +		free_uid(group->inotify_data.user);
> +	}
>  }
>  
>  static void inotify_free_event(struct fsnotify_event *fsn_event)

Re: [PATCH] fsnotify: don't put user context if it was never assigned

[Sasha Levin]

On 09/11/2014 04:43 PM, Andrew Morton wrote:
> On Tue, 29 Jul 2014 09:25:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
> 
>> > On some failure paths we may attempt to free user context even
>> > if it wasn't assigned yet. This will cause a NULL ptr deref
>> > and a kernel BUG.
> Are you able to identify "some failure paths"?  I spent some time
> grepping, but it's a pain.
> 
> Please try to include such info in changelogs because reviewers (ie,
> me) might want to review those callers to decide whether the bug lies
> elsewhere.
> 

Sorry about that.

The path I was looking at is in inotify_new_group():

        oevent = kmalloc(sizeof(struct inotify_event_info), GFP_KERNEL);
        if (unlikely(!oevent)) {
                fsnotify_destroy_group(group);
                return ERR_PTR(-ENOMEM);
        }

fsnotify_destroy_group() would get called here, but group->inotify_data.user
is only getting assigned later:

	group->inotify_data.user = get_current_user();


Thanks,
Sasha