[PATCH] zram: restrict add/remove attributes to root only

[PATCH] zram: restrict add/remove attributes to root only

[Sergey Senozhatsky]

Only root must be able to create a new zram device, therefore
hot_add attribute must have S_IRUSR mode, not S_IRUGO. Otherwise,
anyone can create a new zram device (device initialization with
the disksize attr requires root permission).

Fixes: 6566d1a32bf72 ("zram: add dynamic device add/remove functionality")
Reported-by: Steven Allen <steven@stebalien.com>
Cc: <stable@vger.kernel.org>    [4.2+]
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5163c8f..ee03464 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1414,8 +1414,8 @@ static ssize_t hot_remove_store(struct class *class,
 }
 
 static struct class_attribute zram_control_class_attrs[] = {
-	__ATTR_RO(hot_add),
-	__ATTR_WO(hot_remove),
+	__ATTR(hot_add, 0400, hot_add_show, NULL),
+	__ATTR(hot_remove, 0200, NULL, hot_remove_store),
 	__ATTR_NULL,
 };
 
-- 
2.10.2

Re: [PATCH] zram: restrict add/remove attributes to root only

[Greg KH]

On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
> Only root must be able to create a new zram device, therefore
> hot_add attribute must have S_IRUSR mode, not S_IRUGO. Otherwise,
> anyone can create a new zram device (device initialization with
> the disksize attr requires root permission).
> 
> Fixes: 6566d1a32bf72 ("zram: add dynamic device add/remove functionality")
> Reported-by: Steven Allen <steven@stebalien.com>
> Cc: <stable@vger.kernel.org>    [4.2+]
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 5163c8f..ee03464 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1414,8 +1414,8 @@ static ssize_t hot_remove_store(struct class *class,
>  }
>  
>  static struct class_attribute zram_control_class_attrs[] = {
> -	__ATTR_RO(hot_add),
> -	__ATTR_WO(hot_remove),
> +	__ATTR(hot_add, 0400, hot_add_show, NULL),
> +	__ATTR(hot_remove, 0200, NULL, hot_remove_store),

Huh?  The only change you are making here is that a "normal" user can
not read the value, ATTR_WO only allows root to write to the file.

This change doesn't match up with your changelog, what really is the
problem here with the _RO and _WO values that you can not use them?

Why can't a normal user read the attribute?  Does a read actually modify
something?  If so, that's really not a good idea.

confused,

greg k-h

Re: [PATCH] zram: restrict add/remove attributes to root only

[Sergey Senozhatsky]

On (12/04/16 11:28), Greg KH wrote:
> On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
[..]

> Why can't a normal user read the attribute?  Does a read actually modify
> something?

yes, it does.

reading from a hot_add file creates a new zram device and returns a new
device's device_id. not initialized device (so it does not eat the memory
for handle table, etc.), but with its own set of sysfs attrs, etc. which
consumes memory after all. so a 'normal' user, doing a simple read from a
hot_add file in a loop just for fun, can create a lot of devices and,
quite likely, cause some troubles (as reported by Steven Allen).

	-ss

Re: [PATCH] zram: restrict add/remove attributes to root only

[Greg KH]

On Sun, Dec 04, 2016 at 07:52:08PM +0900, Sergey Senozhatsky wrote:
> On (12/04/16 11:28), Greg KH wrote:
> > On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
> [..]
> 
> > Why can't a normal user read the attribute?  Does a read actually modify
> > something?
> 
> yes, it does.

Oh that's totally and completely broken then.

Reading from a sysfs file should NEVER cause side affects to the system.
Please fix up this api.

> reading from a hot_add file creates a new zram device and returns a new
> device's device_id. not initialized device (so it does not eat the memory
> for handle table, etc.), but with its own set of sysfs attrs, etc. which
> consumes memory after all. so a 'normal' user, doing a simple read from a
> hot_add file in a loop just for fun, can create a lot of devices and,
> quite likely, cause some troubles (as reported by Steven Allen).

Please switch this to be a char device node if you wish to "write and
get a device handle back".  I don't know how I missed that in the
original api review, sorry about that.

For now, you need to document the heck out of this in the attribute
declaration that this is what is going on.  Otherwise someone like me
will come along and "fix up" the file to use ATTR_RO again in the
future and you will have the same problem again.

thanks,

greg k-h

Re: [PATCH] zram: restrict add/remove attributes to root only

[Sergey Senozhatsky]

On (12/04/16 12:28), Greg KH wrote:
> Date: Sun, 4 Dec 2016 12:28:20 +0100
> From: Greg KH <gregkh@linuxfoundation.org>
> To: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>, Minchan Kim
>  <minchan@kernel.org>, Steven Allen <steven@stebalien.com>,
>  linux-kernel@vger.kernel.org, stable@vger.kernel.org, Sergey Senozhatsky
>  <sergey.senozhatsky.work@gmail.com>
> Subject: Re: [PATCH] zram: restrict add/remove attributes to root only
> User-Agent: Mutt/1.7.1 (2016-10-04)
> 
> On Sun, Dec 04, 2016 at 07:52:08PM +0900, Sergey Senozhatsky wrote:
> > On (12/04/16 11:28), Greg KH wrote:
> > > On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
> > [..]
> > 
> > > Why can't a normal user read the attribute?  Does a read actually modify
> > > something?
> > 
> > yes, it does.

to clarify a bit more:

we allocate a new device ID using idr_alloc(). so the IDs are limited
and, thus, the number of devices is limited as well - signed int. each
new device has NO:
 -- zspoll (zsmalloc pool in zram case)
 -- compression per-CPU backends (working-mem/scratch buffers, etc.)
 -- meta table

so no big memory allocations. (a 'normal' user can't init the device,
he/she can just create it. which is the problem here: we don't want a
'normal' user be able to do this).

every device has:
 -- blk queue
 -- sysfs attrs
 -- gendisk
 -- zram structure allocated.

so each new device consumes some memory, but not insane amounts of it.


> Oh that's totally and completely broken then.
> 
> Reading from a sysfs file should NEVER cause side affects to the system.
> Please fix up this api.

some history. we started with a 'loop device'-like scheme, but
ended up with a sysfs approach

 [1] https://marc.info/?l=linux-kernel&m=142495984002611
 [2] https://marc.info/?l=linux-kernel&m=142507747808572
 [3] https://marc.info/?l=linux-kernel&m=142530591720172
 [4] https://marc.info/?l=linux-kernel&m=142509446812318
 [5] https://marc.info/?l=linux-kernel&m=142509782112819


> > reading from a hot_add file creates a new zram device and returns a new
> > device's device_id. not initialized device (so it does not eat the memory
> > for handle table, etc.), but with its own set of sysfs attrs, etc. which
> > consumes memory after all. so a 'normal' user, doing a simple read from a
> > hot_add file in a loop just for fun, can create a lot of devices and,
> > quite likely, cause some troubles (as reported by Steven Allen).
> 
> Please switch this to be a char device node if you wish to "write and
> get a device handle back".  I don't know how I missed that in the
> original api review, sorry about that.
>
> For now, you need to document the heck out of this in the attribute
> declaration that this is what is going on.  Otherwise someone like me
> will come along and "fix up" the file to use ATTR_RO again in the
> future and you will have the same problem again.


I believe we have a documentation

	Documentation/ABI/testing/sysfs-class-zram
and
	Documentation/blockdev/zram.txt

both explain this attr.

	-ss

Re: [PATCH] zram: restrict add/remove attributes to root only

[Greg KH]

On Sun, Dec 04, 2016 at 08:41:17PM +0900, Sergey Senozhatsky wrote:
> On (12/04/16 12:28), Greg KH wrote:
> > Date: Sun, 4 Dec 2016 12:28:20 +0100
> > From: Greg KH <gregkh@linuxfoundation.org>
> > To: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>, Minchan Kim
> >  <minchan@kernel.org>, Steven Allen <steven@stebalien.com>,
> >  linux-kernel@vger.kernel.org, stable@vger.kernel.org, Sergey Senozhatsky
> >  <sergey.senozhatsky.work@gmail.com>
> > Subject: Re: [PATCH] zram: restrict add/remove attributes to root only
> > User-Agent: Mutt/1.7.1 (2016-10-04)
> > 
> > On Sun, Dec 04, 2016 at 07:52:08PM +0900, Sergey Senozhatsky wrote:
> > > On (12/04/16 11:28), Greg KH wrote:
> > > > On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
> > > [..]
> > > 
> > > > Why can't a normal user read the attribute?  Does a read actually modify
> > > > something?
> > > 
> > > yes, it does.
> 
> to clarify a bit more:
> 
> we allocate a new device ID using idr_alloc(). so the IDs are limited
> and, thus, the number of devices is limited as well - signed int. each
> new device has NO:
>  -- zspoll (zsmalloc pool in zram case)
>  -- compression per-CPU backends (working-mem/scratch buffers, etc.)
>  -- meta table
> 
> so no big memory allocations. (a 'normal' user can't init the device,
> he/she can just create it. which is the problem here: we don't want a
> 'normal' user be able to do this).
> 
> every device has:
>  -- blk queue
>  -- sysfs attrs
>  -- gendisk
>  -- zram structure allocated.
> 
> so each new device consumes some memory, but not insane amounts of it.

That's fine, the issue is that reading a file should not cause the
system state to change.  That's just not a logical thing to have happen,
no other sysfs files do that.  Why is zram "special" in this way?

> > Oh that's totally and completely broken then.
> > 
> > Reading from a sysfs file should NEVER cause side affects to the system.
> > Please fix up this api.
> 
> some history. we started with a 'loop device'-like scheme, but
> ended up with a sysfs approach
> 
>  [1] https://marc.info/?l=linux-kernel&m=142495984002611
>  [2] https://marc.info/?l=linux-kernel&m=142507747808572
>  [3] https://marc.info/?l=linux-kernel&m=142530591720172
>  [4] https://marc.info/?l=linux-kernel&m=142509446812318
>  [5] https://marc.info/?l=linux-kernel&m=142509782112819

you should have stuck with the "write a value to the sysfs file" api,
for some reason that didn't stick...

> > > reading from a hot_add file creates a new zram device and returns a new
> > > device's device_id. not initialized device (so it does not eat the memory
> > > for handle table, etc.), but with its own set of sysfs attrs, etc. which
> > > consumes memory after all. so a 'normal' user, doing a simple read from a
> > > hot_add file in a loop just for fun, can create a lot of devices and,
> > > quite likely, cause some troubles (as reported by Steven Allen).
> > 
> > Please switch this to be a char device node if you wish to "write and
> > get a device handle back".  I don't know how I missed that in the
> > original api review, sorry about that.
> >
> > For now, you need to document the heck out of this in the attribute
> > declaration that this is what is going on.  Otherwise someone like me
> > will come along and "fix up" the file to use ATTR_RO again in the
> > future and you will have the same problem again.
> 
> 
> I believe we have a documentation
> 
> 	Documentation/ABI/testing/sysfs-class-zram
> and
> 	Documentation/blockdev/zram.txt
> 
> both explain this attr.

Yes, but that's not in the code itself.  You are doing something VERY
different here than any other sysfs file.  The code better explain it
very well so that I don't go and change this back sometime in the future
when I sweep the kernel for "odd sysfs mode values" like I do every few
years.

So comment this please, why would you object to that?

greg k-h

Re: [PATCH] zram: restrict add/remove attributes to root only

[Sergey Senozhatsky]

On (12/04/16 12:55), Greg KH wrote:
[..]
> That's fine, the issue is that reading a file should not cause the
> system state to change.  That's just not a logical thing to have happen,
> no other sysfs files do that. Why is zram "special" in this way?

yeah, zram is not really special, we just didn't come up with
anything better than that.

> > some history. we started with a 'loop device'-like scheme, but
> > ended up with a sysfs approach
> > 
> >  [1] https://marc.info/?l=linux-kernel&m=142495984002611
> >  [2] https://marc.info/?l=linux-kernel&m=142507747808572
> >  [3] https://marc.info/?l=linux-kernel&m=142530591720172
> >  [4] https://marc.info/?l=linux-kernel&m=142509446812318
> >  [5] https://marc.info/?l=linux-kernel&m=142509782112819
> 
> you should have stuck with the "write a value to the sysfs file" api,
> for some reason that didn't stick...

yes, we had this 'echo ID > /sys/..../zram_add' at some point, but it
didn't fly.

> > I believe we have a documentation
> > 
> > 	Documentation/ABI/testing/sysfs-class-zram
> > and
> > 	Documentation/blockdev/zram.txt
> > 
> > both explain this attr.
> 
> Yes, but that's not in the code itself.  You are doing something VERY
> different here than any other sysfs file.  The code better explain it
> very well so that I don't go and change this back sometime in the future
> when I sweep the kernel for "odd sysfs mode values" like I do every few
> years.
> 
> So comment this please, why would you object to that?

oh, I'm not objecting. I just gave as much info as possible.
v2 [with a comment]  will be out soon.


will this comment suffice?

====

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ee03464..3a0576f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1413,6 +1413,13 @@ static ssize_t hot_remove_store(struct class *class,
        return ret ? ret : count;
 }
 
+/*
+ * NOTE: hot_add attribute is not the usual read-only sysfs
+ * attribute. In a sence that reading from this file does alter
+ * the state of your system -- it creates a new un-initialized
+ * zram device and returns back this device's device_id (or an
+ * error code if it fails to create a new device).
+ */
 static struct class_attribute zram_control_class_attrs[] = {
        __ATTR(hot_add, 0400, hot_add_show, NULL),
        __ATTR(hot_remove, 0200, NULL, hot_remove_store),

--

	-ss

Re: [PATCH] zram: restrict add/remove attributes to root only

[Sergey Senozhatsky]

On (12/04/16 12:28), Greg KH wrote:
> > [..]
> > 
> > > Why can't a normal user read the attribute?  Does a read actually modify
> > > something?
> > 
> > yes, it does.
> 
> Oh that's totally and completely broken then.
> 
> Reading from a sysfs file should NEVER cause side affects to the system.
> Please fix up this api.

we have a mechanism for such API changes -- there is a bunch of deprecated
sysfs attrs that we will remove in 4.11; so I'll mark hot_add/hot_remove
as deprecated and switch to char device (as you suggested). thanks.

	-ss