Re: [PATCH] param: do not set store func without write perm

[Rusty Russell]

Kees Cook <keescook@xxxxxxxxxxxx> writes:
> When a module_param is defined without DAC write permissions, it can
> still be changed at runtime and updated. Drivers using a 0444 permission
> may be surprised that these values can still be changed.
>
> For drivers that want to allow updates, any S_IW* flag will set the
> "store" function as before. Drivers without S_IW* flags will have the
> "store" function unset, unforcing a read-only value. Drivers that wish
> neither "store" nor "get" can continue to use "0" for perms to stay out
> of sysfs entirely.

Hmm, fair enough.  The use of the acronym DAC here is a bit weird;
I would have just said:

	/* If no perms, it's not writable even if root chmods it! */
	if ((kp->perm & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0)
		new->attrs[num].mattr.store = param_attr_store;

Applied (with fuzz fixed, since I reworked that code in my tree).

Thanks,
Rusty.

>
> Old behavior:
>   # cd /sys/module/snd/parameters
>   # ls -l
>   total 0
>   -r--r--r-- 1 root root 4096 Dec 11 13:55 cards_limit
>   -r--r--r-- 1 root root 4096 Dec 11 13:55 major
>   -r--r--r-- 1 root root 4096 Dec 11 13:55 slots
>   # cat major
>   116
>   # echo -1 > major
>   -bash: major: Permission denied
>   # chmod u+w major
>   # echo -1 > major
>   # cat major
>   -1
>
> New behavior:
>   ...
>   # chmod u+w major
>   # echo -1 > major
>   -bash: echo: write error: Input/output error
>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  kernel/params.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index db97b791390f..fd50ce9f1bbf 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -647,7 +647,9 @@ static __modinit int add_sysfs_param(struct module_kobject *mk,
>  	sysfs_attr_init(&new->attrs[num].mattr.attr);
>  	new->attrs[num].param = kp;
>  	new->attrs[num].mattr.show = param_attr_show;
> -	new->attrs[num].mattr.store = param_attr_store;
> +	/* Do not allow runtime DAC changes to make param writable. */
> +	if ((kp->perm & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0)
> +		new->attrs[num].mattr.store = param_attr_store;
>  	new->attrs[num].mattr.attr.name = (char *)name;
>  	new->attrs[num].mattr.attr.mode = kp->perm;
>  	new->num = num+1;
> -- 
> 1.9.1
>
>
> -- 
> Kees Cook
> Chrome OS Security
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/