linux-kernel - Re: [PATCH v4 6/8] x86: bump ZO_z_extra

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]

Message-ID: <D45F637D-6BB0-4F08-BEBE-FAB9B56F36F6@fb.com>
Date:   Wed, 1 Apr 2020 17:33:03 +0000
From:   Nick Terrell <terrelln@...com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Nick Terrell <nickrterrell@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chris Mason <clm@...com>,
        "linux-kbuild@...r.kernel.org" <linux-kbuild@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Petr Malat <oss@...at.biz>, Kees Cook <keescook@...omium.org>,
        Kernel Team <Kernel-team@...com>,
        Adam Borowski <kilobyte@...band.pl>,
        Patrick Williams <patrickw3@...com>,
        "Michael van der Westhuizen" <rmikey@...com>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "Patrick Williams" <patrick@...cx.xyz>,
        Sedat Dilek <sedat.dilek@...il.com>
Subject: Re: [PATCH v4 6/8] x86: bump ZO_z_extra_bytes margin for zstd



> On Apr 1, 2020, at 2:33 AM, Borislav Petkov <bp@...en8.de> wrote:
> 
> On Tue, Mar 31, 2020 at 10:39:11PM -0700, Nick Terrell wrote:
>> From: Nick Terrell <terrelln@...com>
>> 
>> Bump the ZO_z_extra_bytes margin for zstd.
>> 
>> Zstd needs 3 bytes per 128 KB, and has a 22 byte fixed overhead.
>> Zstd needs to maintain 128 KB of space at all times, since that is
>> the maximum block size. See the comments regarding in-place
>> decompression added in lib/decompress_unzstd.c for details.
>> 
>> Reviewed-by: Kees Cook <keescook@...omium.org>
>> Tested-by: Sedat Dilek <sedat.dilek@...il.com>
>> Signed-off-by: Nick Terrell <terrelln@...com>
>> ---
>> arch/x86/boot/header.S | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> index 97d9b6d6c1af..b820875c5c95 100644
>> --- a/arch/x86/boot/header.S
>> +++ b/arch/x86/boot/header.S
>> @@ -536,8 +536,14 @@ pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred load addr
>> # the size-dependent part now grows so fast.
>> #
>> # extra_bytes = (uncompressed_size >> 8) + 65536
>> +#
>> +# ZSTD compressed data grows by at most 3 bytes per 128K, and only has a 22
>> +# byte fixed overhead but has a maximum block size of 128K, so it needs a
>> +# larger margin.
>> +#
>> +# extra_bytes = (uncompressed_size >> 8) + 131072
>> 
>> -#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 65536)
>> +#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 131072)
>> #if ZO_z_output_len > ZO_z_input_len
>> # define ZO_z_extract_offset	(ZO_z_output_len + ZO_z_extra_bytes - \
>> 				 ZO_z_input_len)
>> -- 
> 
> So why is this change unconditional if only this compression alg. needs
> it?

The code is currently written so that all the compression algorithms use the
same ZO_z_extra_bytes. It is taken to be the maximum of the growth rate
plus the maximum fixed overhead. Just a few lines above is the comment:

# … Hence safety
# margin should be updated to cover all decompressors so that we don't
# need to deal with each of them separately. Please check
# the description in lib/decompressor_xxx.c for specific information.

So I was been following the guidance in the comments. If we want to refactor
it to handle each compressor individually we could make ZO_z_extra_bytes
smaller for most algorithms.

Does it matter? I’m not an expert here, but it seems to me that requiring an
extra 64 KB of RAM for kernel decompression isn’t such an onerous addition.

Best,
Nick