[PATCH] sysfs: check if one entry has been removed before freeing

[PATCH] sysfs: check if one entry has been removed before freeing

[Ming Lei]

It might be a kernel disaster if one sysfs entry is freed but
still referenced by sysfs tree.

Recently Dave and Sasha reported one use-after-free problem on
sysfs entry, and the problem has been troubleshooted with help
of debug message added in this patch.

Given sysfs_get_dirent/sysfs_put are exported APIs, even inside
sysfs they are called in many contexts(kobject/attribe add/delete,
inode init/drop, dentry lookup/release, readdir, ...), it is healthful
to check the removed flag before freeing one entry and dump message
if it is freeing without being removed first.

Cc: Dave Jones <davej@redhat.com>
Cc: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 fs/sysfs/dir.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 1bf016b..328ef9b 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
 	 */
 	parent_sd = sd->s_parent;
 
+	if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
+		printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
+			parent_sd ? parent_sd->s_name : "",
+			sd->s_name);
+		BUG();
+	}
+
 	if (sysfs_type(sd) == SYSFS_KOBJ_LINK)
 		sysfs_put(sd->s_symlink.target_sd);
 	if (sysfs_type(sd) & SYSFS_COPY_NAME)
@@ -386,7 +393,7 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
 
 	sd->s_name = name;
 	sd->s_mode = mode;
-	sd->s_flags = type;
+	sd->s_flags = type | SYSFS_FLAG_REMOVED;
 
 	return sd;
 
@@ -466,6 +473,9 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 		ps_iattrs->ia_ctime = ps_iattrs->ia_mtime = CURRENT_TIME;
 	}
 
+	/* Mark the entry added into directory tree */
+	sd->s_flags &= ~SYSFS_FLAG_REMOVED;
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Dave Jones]

On Wed, Apr 03, 2013 at 10:58:23AM +0800, Ming Lei wrote:
 
 > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
 > index 1bf016b..328ef9b 100644
 > --- a/fs/sysfs/dir.c
 > +++ b/fs/sysfs/dir.c
 > @@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
 >  	 */
 >  	parent_sd = sd->s_parent;
 >  
 > +	if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
 > +		printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
 > +			parent_sd ? parent_sd->s_name : "",
 > +			sd->s_name);
 > +		BUG();
 > +	}

Please use WARN instead of BUG.  For an in-ram filesystem like
sysfs, there's no real reason to lock-up the machine in this way
making it harder to debug.

	Dave

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Ming Lei]

On Wed, Apr 3, 2013 at 11:04 AM, Dave Jones <davej@redhat.com> wrote:
> On Wed, Apr 03, 2013 at 10:58:23AM +0800, Ming Lei wrote:
>
>  > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>  > index 1bf016b..328ef9b 100644
>  > --- a/fs/sysfs/dir.c
>  > +++ b/fs/sysfs/dir.c
>  > @@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
>  >       */
>  >      parent_sd = sd->s_parent;
>  >
>  > +    if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
>  > +            printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
>  > +                    parent_sd ? parent_sd->s_name : "",
>  > +                    sd->s_name);
>  > +            BUG();
>  > +    }
>
> Please use WARN instead of BUG.  For an in-ram filesystem like
> sysfs, there's no real reason to lock-up the machine in this way
> making it harder to debug.

If WARN is used, the freed memory will be allocated to other
kernel components, then sysfs may change the memory and cause
destruction, so maybe it is better to use BUG to stop kernel.

Thanks,
--
Ming Lei

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Greg Kroah-Hartman]

On Wed, Apr 03, 2013 at 11:52:39AM +0800, Ming Lei wrote:
> On Wed, Apr 3, 2013 at 11:04 AM, Dave Jones <davej@redhat.com> wrote:
> > On Wed, Apr 03, 2013 at 10:58:23AM +0800, Ming Lei wrote:
> >
> >  > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> >  > index 1bf016b..328ef9b 100644
> >  > --- a/fs/sysfs/dir.c
> >  > +++ b/fs/sysfs/dir.c
> >  > @@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
> >  >       */
> >  >      parent_sd = sd->s_parent;
> >  >
> >  > +    if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
> >  > +            printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
> >  > +                    parent_sd ? parent_sd->s_name : "",
> >  > +                    sd->s_name);
> >  > +            BUG();
> >  > +    }
> >
> > Please use WARN instead of BUG.  For an in-ram filesystem like
> > sysfs, there's no real reason to lock-up the machine in this way
> > making it harder to debug.
> 
> If WARN is used, the freed memory will be allocated to other
> kernel components, then sysfs may change the memory and cause
> destruction, so maybe it is better to use BUG to stop kernel.

No, it's never ok to call BUG(), sorry, please fix this.

greg k-h

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Ming Lei]

On Wed, Apr 3, 2013 at 1:35 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Apr 03, 2013 at 11:52:39AM +0800, Ming Lei wrote:
>> On Wed, Apr 3, 2013 at 11:04 AM, Dave Jones <davej@redhat.com> wrote:
>> > On Wed, Apr 03, 2013 at 10:58:23AM +0800, Ming Lei wrote:
>> >
>> >  > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>> >  > index 1bf016b..328ef9b 100644
>> >  > --- a/fs/sysfs/dir.c
>> >  > +++ b/fs/sysfs/dir.c
>> >  > @@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
>> >  >       */
>> >  >      parent_sd = sd->s_parent;
>> >  >
>> >  > +    if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
>> >  > +            printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
>> >  > +                    parent_sd ? parent_sd->s_name : "",
>> >  > +                    sd->s_name);
>> >  > +            BUG();
>> >  > +    }
>> >
>> > Please use WARN instead of BUG.  For an in-ram filesystem like
>> > sysfs, there's no real reason to lock-up the machine in this way
>> > making it harder to debug.
>>
>> If WARN is used, the freed memory will be allocated to other
>> kernel components, then sysfs may change the memory and cause
>> destruction, so maybe it is better to use BUG to stop kernel.
>
> No, it's never ok to call BUG(), sorry, please fix this.

Sorry, could you explain it in a bit detail? IMO, it is really a bug
when code runs here, and there are much similar BUG_ON()
uses in current sysfs code too.

Thanks,
--
Ming Lei

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Greg Kroah-Hartman]

On Wed, Apr 03, 2013 at 03:05:37PM +0800, Ming Lei wrote:
> On Wed, Apr 3, 2013 at 1:35 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Apr 03, 2013 at 11:52:39AM +0800, Ming Lei wrote:
> >> On Wed, Apr 3, 2013 at 11:04 AM, Dave Jones <davej@redhat.com> wrote:
> >> > On Wed, Apr 03, 2013 at 10:58:23AM +0800, Ming Lei wrote:
> >> >
> >> >  > diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> >> >  > index 1bf016b..328ef9b 100644
> >> >  > --- a/fs/sysfs/dir.c
> >> >  > +++ b/fs/sysfs/dir.c
> >> >  > @@ -268,6 +268,13 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
> >> >  >       */
> >> >  >      parent_sd = sd->s_parent;
> >> >  >
> >> >  > +    if (unlikely(!(sd->s_flags & SYSFS_FLAG_REMOVED))) {
> >> >  > +            printk(KERN_ERR "sysfs: free using entry: %s/%s\n",
> >> >  > +                    parent_sd ? parent_sd->s_name : "",
> >> >  > +                    sd->s_name);
> >> >  > +            BUG();
> >> >  > +    }
> >> >
> >> > Please use WARN instead of BUG.  For an in-ram filesystem like
> >> > sysfs, there's no real reason to lock-up the machine in this way
> >> > making it harder to debug.
> >>
> >> If WARN is used, the freed memory will be allocated to other
> >> kernel components, then sysfs may change the memory and cause
> >> destruction, so maybe it is better to use BUG to stop kernel.
> >
> > No, it's never ok to call BUG(), sorry, please fix this.
> 
> Sorry, could you explain it in a bit detail? IMO, it is really a bug
> when code runs here, and there are much similar BUG_ON()
> uses in current sysfs code too.

Then make it a WARN() call, like David said, to give us a chance to get
the report from a user so we can fix it.  If the machine crashes after
that, fine, but hopefully we will get a oops report out of it.

greg k-h

Re: [PATCH] sysfs: check if one entry has been removed before freeing

[Ming Lei]

On Thu, Apr 4, 2013 at 12:08 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Then make it a WARN() call, like David said, to give us a chance to get
> the report from a user so we can fix it.  If the machine crashes after
> that, fine, but hopefully we will get a oops report out of it.

OK, got it, and console might not be available at that time.


Thanks,
--
Ming Lei