From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752601AbdI1It5 (ORCPT ); Thu, 28 Sep 2017 04:49:57 -0400 Received: from mx2.suse.de ([195.135.220.15]:46400 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752186AbdI1Itz (ORCPT ); Thu, 28 Sep 2017 04:49:55 -0400 Date: Thu, 28 Sep 2017 10:49:53 +0200 From: Jean Delvare To: LKML Cc: Andrew Morton , Ingo Molnar , Baoquan He Subject: Re: [PATCH] params: Fix an overflow in param_attr_show Message-ID: <20170928104953.0665a9f0@endymion> In-Reply-To: <20170927101031.7a3b2398@endymion> References: <20170927101031.7a3b2398@endymion> Organization: SUSE Linux X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.31; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 27 Sep 2017 10:10:31 +0200, 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(-) > (...) This patch turns out to be broken, so please disregard for now. I missed several types, like bool, invbool and string (not sure why we have types charp and string which seems to serve the same purpose...) and most importantly arrays of parameters, which very much assume that individual parameters are not \n-terminated. There had to be a reason for the current code... Sorry for the noise, I'll work some more on it and see how to address all the remaining issues. -- Jean Delvare SUSE L3 Support