linux-kernel - Re: [PATCH 1/9] crypto: add zbufsize() interface

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, 2 Dec 2021 00:10:13 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Geliang Tang <geliangtang@...il.com>,
        Arnd Bergmann <arnd@...db.de>, Haren Myneni <haren@...ibm.com>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        Zhou Wang <wangzhou1@...ilicon.com>,
        linux-crypto <linux-crypto@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH 1/9] crypto: add zbufsize() interface

On Thu, Dec 02, 2021 at 02:57:27PM +1100, Herbert Xu wrote:
> On Wed, Dec 01, 2021 at 07:51:25PM -0800, Kees Cook wrote:
> >
> > But the scomp API appears to be "internal only":
> > 
> > include/crypto/internal/scompress.h:static inline int crypto_scomp_decompress(struct crypto_scomp *tfm,
> > 
> > What's the correct API calling sequence to do a simple decompress?
> 
> OK we haven't wired up scomp to users because there was no user
> to start with.  So if you like you can create it just as we did
> for shash.
> 
> The question becomes do you want to restrict your use-case to
> synchronous-only algorithms, i.e., you will never be able to access
> offload devices that support compression?

I'd rather just have a simple API that hid all the async (or sync) details
and would work with whatever was the "best" implementation. Neither pstore
nor the module loader has anything else to do while decompression happens.

> Typically this would only make sense if you process a very small
> amount of data, but this seems counter-intuitive with compression
> (it does make sense with hashing where we often hash just 16 bytes).

pstore works on usually a handful of small buffers. (One of the largest
I've seen is used by Chrome OS: 6 128K buffers.) Speed is not important
(done at most 6 times at boot, and 1 time on panic), and, in fact,
offload is probably a bad idea just to keep the machinery needed to
store a panic log as small as possible.

The module loader is also doing non-fast-path decompression of modules,
with each of those being maybe a couple megabytes. This isn't fast-path
either: if it's not the kernel, it'd be userspace doing the decompression,
and it only happens once per module, usually at boot.

Why can't crypto_comp_*() be refactored to wrap crypto_acomp_*() (and
crypto_scomp_*())? I can see so many other places that would benefit from
this. Here are just some of the places that appear to be hand-rolling
compression/decompression routines that might benefit from this kind of
code re-use and compression alg agnosticism:

fs/pstore/platform.c
drivers/gpu/drm/i915/i915_gpu_error.c
kernel/power/swap.c
arch/powerpc/kernel/nvram_64.c
security/apparmor/policy_unpack.c
drivers/base/regmap/regcache-lzo.c
fs/btrfs/lzo.c
fs/btrfs/zlib.c
fs/f2fs/compress.c
fs/jffs2/compr_lzo.c
drivers/net/ethernet/chelsio/cxgb4/cudbg_zlib.h
drivers/net/ppp/ppp_deflate.c
fs/jffs2/compr_lzo.c
fs/jffs2/compr_zlib.c

But right now there isn't a good way to just do a simple one-off:

	dst = decompress_named(alg_name, dst, dst_len, src, src_len);

or if it happens more than once:

	alg = compressor(alg_name);
	set_comp_alg_param(param, value);
        ...
        for (...) {
		...
		dst = compress(alg, dst, dst_len, src, src_len);
		...
	}
        ...
        free_compressor(alg);

-Kees

-- 
Kees Cook