linux-kernel - Re: [PATCH] at24: make module parameters changeable via sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120914102536.1ebd248d@endymion.delvare>
Date:	Fri, 14 Sep 2012 10:25:36 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
Cc:	Wolfram Sang <w.sang@...gutronix.de>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH] at24: make module parameters changeable via sysfs

Hi Uwe,

On Wed, 12 Sep 2012 11:43:32 +0200, Uwe Kleine-König wrote:
> The respective values are evaluated at each read/write, so no further
> action is required than to change the perm argument to module_param.
> 
> Note there is no sanity check so root can make the driver effectively
> unusable but that's what root is for :-)
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> ---
>  drivers/misc/eeprom/at24.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index ab1ad41..8a5a192 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -85,7 +85,7 @@ struct at24_data {
>   * This value is forced to be a power of two so that writes align on pages.
>   */
>  static unsigned io_limit = 128;
> -module_param(io_limit, uint, 0);
> +module_param(io_limit, uint, S_IRUGO | S_IWUSR);

This won't work. Not only there is no validation of the value, while
there is such a validation (and value adjustment!) in at24_init(); you
seem to not care, but I do. But the more important problem is that
changing io_limit at run-time will only affect reads, not writes. The
size limit from writes is computed at device probing time:

static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
(...)
	if (writable) {
	(...)
			if (write_max > io_limit)
				write_max = io_limit;

So changing the value through sysfs will have no effect. If you want it
to have an effect, you have to move the check from at24_probe() to
at24_eeprom_write().

Back to the validation issue, I think it would be worth looking into
module_param_cb(). Using it, it may not be that difficult to get
validation when the value is changed through sysfs. Otherwise I'll ask
you to check what exactly happens if someone sets io_limit to 0. We
can't afford infinite loops or EEPROM corruption on root mistyping.

>  MODULE_PARM_DESC(io_limit, "Maximum bytes per I/O (default 128)");
>  
>  /*
> @@ -93,7 +93,7 @@ MODULE_PARM_DESC(io_limit, "Maximum bytes per I/O (default 128)");
>   * it's important to recover from write timeouts.
>   */
>  static unsigned write_timeout = 25;
> -module_param(write_timeout, uint, 0);
> +module_param(write_timeout, uint, S_IRUGO | S_IWUSR);

This one is OK.

>  MODULE_PARM_DESC(write_timeout, "Time (in ms) to try writes (default 25)");
>  
>  #define AT24_SIZE_BYTELEN 5


-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/