Hi all, It was brought to my attention by a customer that 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 doesn't look 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.)
One way to fix it would be to modify the scnprintf calls in kernel/params.c to consider the buffer size as PAGE_SIZE - 1: --- linux-4.13.orig/kernel/params.c 2017-09-19 16:20:02.965748249 +0200 +++ linux-4.13/kernel/params.c 2017-09-19 16:20:47.723374030 +0200 @@ -224,7 +224,7 @@ char *parse_args(const char *doing, } \ int param_get_##name(char *buffer, const struct kernel_param *kp) \ { \ - return scnprintf(buffer, PAGE_SIZE, format, \ + return scnprintf(buffer, PAGE_SIZE - 1, format, \ *((type *)kp->arg)); \ } \ const struct kernel_param_ops param_ops_##name = { \ @@ -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 - 1, "%s", *((char **)kp->arg)); } EXPORT_SYMBOL(param_get_charp); This guarantees that we always have enough room to add the trailing '\n'. However my impression is that seq_file doesn't care about strings being NUL-terminated, all it sees is a buffer and a byte count. So we might as well just replace the trailing '\0' with a '\n' and not add another '\0' after that. This nicely avoids the buffer overflow, and is also faster than calling strcat(): --- linux-4.13.orig/kernel/params.c 2017-07-03 01:07:02.000000000 +0200 +++ linux-4.13/kernel/params.c 2017-09-19 15:20:28.870798377 +0200 @@ -550,8 +550,8 @@ static ssize_t param_attr_show(struct mo count = attribute->param->ops->get(buf, attribute->param); kernel_param_unlock(mk->mod); if (count > 0) { - strcat(buf, "\n"); - ++count; + /* Replace the terminating NUL by a newline */ + buf[count++] = '\n'; } return count; } A quick test did not reveal any problem. Yet another option would be to add the trailing '\n' when writing to the buffer originally: --- 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; } The only case where it makes a difference is for empty string parameters. They are rare but they exist (for example parameter edid_firmware of module drm_kms_helper is an empty string by default.) Reading such parameters would now return a blank line, instead of nothing at all. I think it is more consistent, more efficient and more elegant, but I don't know if anyone is worried about potential side effects? If so, this special case could also be handled directly in function param_get_charp. I would personally prefer the 3rd option, but I'm curious if other developers have different opinions (and if so, why.) Thanks, -- Jean Delvare SUSE L3 Support