* Jean Delvare <jdelv...@suse.de> wrote:
> Function param_attr_show could overflow the buffer it is operating
> on. The buffer size is PAGE_SIZE, and the string returned by
> attribute->param->ops->get is generated by scnprintf(buffer,
> PAGE_SIZE, ...) so it could be PAGE_SIZE - 1 long, with the
> terminating '\0' at the very end of the buffer. Calling
> strcat(..., "\n") on this isn't safe, as the '\0' will be replaced
> by '\n' (OK) and then another '\0' will be added past the end of
> the buffer (not OK.)
>
> Simply add the trailing '\n' when writing the attribute contents to
> the buffer originally. This is safe, and also faster.
>
> Credits to Teradata for discovering this issue.
>
> Signed-off-by: Jean Delvare <jdelv...@suse.de>
> ---
> kernel/params.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> --- linux-4.13.orig/kernel/params.c 2017-09-19 16:07:18.794254776 +0200
> +++ linux-4.13/kernel/params.c 2017-09-19 16:12:57.398426205 +0200
> @@ -236,14 +236,14 @@ char *parse_args(const char *doing,
> EXPORT_SYMBOL(param_ops_##name)
>
>
> -STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
> -STANDARD_PARAM_DEF(short, short, "%hi", kstrtos16);
> -STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", kstrtou16);
> -STANDARD_PARAM_DEF(int, int, "%i", kstrtoint);
> -STANDARD_PARAM_DEF(uint, unsigned int, "%u", kstrtouint);
> -STANDARD_PARAM_DEF(long, long, "%li", kstrtol);
> -STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", kstrtoul);
> -STANDARD_PARAM_DEF(ullong, unsigned long long, "%llu", kstrtoull);
> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8);
> +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16);
> +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16);
> +STANDARD_PARAM_DEF(int, int, "%i\n", kstrtoint);
> +STANDARD_PARAM_DEF(uint, unsigned int, "%u\n", kstrtouint);
> +STANDARD_PARAM_DEF(long, long, "%li\n", kstrtol);
> +STANDARD_PARAM_DEF(ulong, unsigned long, "%lu\n", kstrtoul);
> +STANDARD_PARAM_DEF(ullong, unsigned long long, "%llu\n", kstrtoull);
>
> int param_set_charp(const char *val, const struct kernel_param *kp)
> {
> @@ -270,7 +270,7 @@ EXPORT_SYMBOL(param_set_charp);
>
> int param_get_charp(char *buffer, const struct kernel_param *kp)
> {
> - return scnprintf(buffer, PAGE_SIZE, "%s", *((char **)kp->arg));
> + return scnprintf(buffer, PAGE_SIZE, "%s\n", *((char **)kp->arg));
> }
> EXPORT_SYMBOL(param_get_charp);
>
> @@ -549,10 +549,6 @@ static ssize_t param_attr_show(struct mo
> kernel_param_lock(mk->mod);
> count = attribute->param->ops->get(buf, attribute->param);
> kernel_param_unlock(mk->mod);
> - if (count > 0) {
> - strcat(buf, "\n");
> - ++count;
> - }
> return count;
> }
So the \n additions to the STANDARD_PARAM_DEF() lines
> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8);
> +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16);
> +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16);
are not necessary anymore, with the other changes? If so then I'd leave them
without the \n - that's also easier to read.
Or if adding this:
STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
... is still unsafe then I'd suggest making it safe - it's easy to miss the
lack
of a \n during review and testing.
Thanks,
Ingo
