* [PATCH] fsnotify: don't put user context if it was never assigned
@ 2014-07-29 13:25 Sasha Levin
2014-09-03 21:31 ` Sasha Levin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sasha Levin @ 2014-07-29 13:25 UTC (permalink / raw)
To: john, rlove, eparis; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-07-29 13:25 [PATCH] fsnotify: don't put user context if it was never assigned Sasha Levin
@ 2014-09-03 21:31 ` Sasha Levin
2014-09-08 14:24 ` Sasha Levin
2014-09-11 17:52 ` Heinrich Schuchardt
2014-09-11 20:43 ` Andrew Morton
2 siblings, 1 reply; 7+ messages in thread
From: Sasha Levin @ 2014-09-03 21:31 UTC (permalink / raw)
To: Sasha Levin, john, rlove, eparis; +Cc: linux-kernel
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)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-09-03 21:31 ` Sasha Levin
@ 2014-09-08 14:24 ` Sasha Levin
0 siblings, 0 replies; 7+ messages in thread
From: Sasha Levin @ 2014-09-08 14:24 UTC (permalink / raw)
To: john, rlove, eparis; +Cc: linux-kernel, Al Viro, Linus Torvalds
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)
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-07-29 13:25 [PATCH] fsnotify: don't put user context if it was never assigned Sasha Levin
2014-09-03 21:31 ` Sasha Levin
@ 2014-09-11 17:52 ` Heinrich Schuchardt
2014-09-12 16:20 ` Jan Kara
2014-09-11 20:43 ` Andrew Morton
2 siblings, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2014-09-11 17:52 UTC (permalink / raw)
To: Sasha Levin; +Cc: john, rlove, eparis, linux-kernel, Jan Kara, Andrew Morton
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)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-07-29 13:25 [PATCH] fsnotify: don't put user context if it was never assigned Sasha Levin
2014-09-03 21:31 ` Sasha Levin
2014-09-11 17:52 ` Heinrich Schuchardt
@ 2014-09-11 20:43 ` Andrew Morton
2014-09-12 1:49 ` Sasha Levin
2 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2014-09-11 20:43 UTC (permalink / raw)
To: Sasha Levin; +Cc: john, rlove, eparis, linux-kernel
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)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-09-11 20:43 ` Andrew Morton
@ 2014-09-12 1:49 ` Sasha Levin
0 siblings, 0 replies; 7+ messages in thread
From: Sasha Levin @ 2014-09-12 1:49 UTC (permalink / raw)
To: Andrew Morton; +Cc: john, rlove, eparis, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fsnotify: don't put user context if it was never assigned
2014-09-11 17:52 ` Heinrich Schuchardt
@ 2014-09-12 16:20 ` Jan Kara
0 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2014-09-12 16:20 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Sasha Levin, john, rlove, eparis, linux-kernel, Jan Kara, Andrew Morton
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-12 16:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-29 13:25 [PATCH] fsnotify: don't put user context if it was never assigned Sasha Levin
2014-09-03 21:31 ` Sasha Levin
2014-09-08 14:24 ` Sasha Levin
2014-09-11 17:52 ` Heinrich Schuchardt
2014-09-12 16:20 ` Jan Kara
2014-09-11 20:43 ` Andrew Morton
2014-09-12 1:49 ` Sasha Levin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.