linux-kernel - Re: [PATCH] params: Fix an overflow in param_attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Sep 2017 10:49:53 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     LKML <linux-kernel@...r.kernel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>, Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH] params: Fix an overflow in param_attr_show

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 <jdelvare@...e.de>
> ---
>  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