netdev - Re: [RFC PATCH 05/21] ubifs: Pass worst-case buffer size to compression routines

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: <4fc9930e-152b-1de1-9532-d1eefa6c277c@huawei.com>
Date: Thu, 20 Jul 2023 09:23:31 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: Ard Biesheuvel <ardb@...nel.org>
CC: Eric Biggers <ebiggers@...nel.org>, <linux-crypto@...r.kernel.org>,
	Herbert Xu <herbert@...dor.apana.org.au>, Kees Cook <keescook@...omium.org>,
	Haren Myneni <haren@...ibm.com>, Nick Terrell <terrelln@...com>, Minchan Kim
	<minchan@...nel.org>, Sergey Senozhatsky <senozhatsky@...omium.org>, Jens
 Axboe <axboe@...nel.dk>, Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
	Richard Weinberger <richard@....at>, David Ahern <dsahern@...nel.org>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Steffen Klassert <steffen.klassert@...unet.com>,
	<linux-kernel@...r.kernel.org>, <linux-block@...r.kernel.org>,
	<qat-linux@...el.com>, <linuxppc-dev@...ts.ozlabs.org>,
	<linux-mtd@...ts.infradead.org>, <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH 05/21] ubifs: Pass worst-case buffer size to
 compression routines

在 2023/7/19 22:38, Ard Biesheuvel 写道:
> On Wed, 19 Jul 2023 at 16:23, Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>>
>> 在 2023/7/19 16:33, Ard Biesheuvel 写道:
>>> On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@...nel.org> wrote:
>>>>
>>>> On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote:
>>>>> Currently, the ubifs code allocates a worst case buffer size to
>>>>> recompress a data node, but does not pass the size of that buffer to the
>>>>> compression code. This means that the compression code will never use
>>
>> I think you mean the 'out_len' which describes the lengh of 'buf' is
>> passed into ubifs_decompress, which effects the result of
>> decompressor(eg. lz4 uses length to calculate the buffer end pos).
>> So, we should pass the real lenghth of 'buf'.
>>
> 
> Yes, that is what I meant.
> 
> But Eric makes a good point, and looking a bit more closely, there is
> really no need for the multiplication here: we know the size of the
> decompressed data, so we don't need the additional space.
> 

Right, we get 'out_len' from 'dn->size' which is the length of 
uncompressed data. ubifs_compress makes sure the compressed length is 
smaller than original length.

> I intend to drop this patch, and replace it with the following:
> 
> ----------------8<--------------
> 
> Currently, when truncating a data node, a decompression buffer is
> allocated that is twice the size of the data node's uncompressed size.
> However, the fact that this space is available is not communicated to
> the compression routines, as out_len itself is not updated.
> 
> The additional space is not needed even in the theoretical worst case
> where compression might lead to inadvertent expansion: first of all,
> increasing the size of the input buffer does not help mitigate that
> issue. And given the truncation of the data node and the fact that the
> original data compressed well enough to pass the UBIFS_MIN_COMPRESS_DIFF
> test, there is no way on this particular code path that compression
> could result in expansion beyond the original decompressed size, and so
> no mitigation is necessary to begin with.
> 
> So let's just drop WORST_COMPR_FACTOR here.
> 
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> 
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index dc52ac0f4a345f30..0b55cbfe0c30505e 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -1489,7 +1489,7 @@ static int truncate_data_node(const struct
> ubifs_info *c, const struct inode *in
>          int err, dlen, compr_type, out_len, data_size;
> 
>          out_len = le32_to_cpu(dn->size);
> -       buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
> +       buf = kmalloc(out_len, GFP_NOFS);
>          if (!buf)
>                  return -ENOMEM;
> .
> 

This version looks better.

Reviewed-by: Zhihao Cheng <chengzhihao1@...wei.com>