From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752499AbdI0I0u (ORCPT ); Wed, 27 Sep 2017 04:26:50 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:35010 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752100AbdI0I0q (ORCPT ); Wed, 27 Sep 2017 04:26:46 -0400 X-Google-Smtp-Source: AOwi7QBbYjs7gMGXYFlwDi+/9ltlVkXn5x8cbU9lNgPxncbNZOiasWmSbTiN/wHou5kUuxMtV1ujqA== Date: Wed, 27 Sep 2017 10:26:42 +0200 From: Ingo Molnar To: Jean Delvare Cc: LKML , Andrew Morton , Baoquan He , Linus Torvalds , Thomas Gleixner , Peter Zijlstra , "H. Peter Anvin" , Borislav Petkov Subject: Re: [PATCH] params: Fix an overflow in param_attr_show Message-ID: <20170927082642.slh2gk3zuw5j7gmh@gmail.com> References: <20170927101031.7a3b2398@endymion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170927101031.7a3b2398@endymion> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Jean Delvare 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 > --- > 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