Re: [PATCH] tools/xg: increase LZMA_BLOCK_SIZE for uncompressing the kernel

[Jürgen Groß <jgross@xxxxxxxx>]

On 09.10.24 13:15, Jan Beulich wrote:
> On 09.10.2024 13:08, Andrew Cooper wrote:
> > On 09/10/2024 11:26 am, Juergen Gross wrote:
> > > On 09.10.24 12:19, Jan Beulich wrote:
> > > > On 09.10.2024 11:52, Marek Marczykowski-Górecki wrote:
> > > > > On Wed, Oct 09, 2024 at 09:19:57AM +0200, Jan Beulich wrote:
> > > > > > On 08.10.2024 23:32, Marek Marczykowski-Górecki wrote:
> > > > > > > --- a/tools/libs/guest/xg_dom_bzimageloader.c
> > > > > > > +++ b/tools/libs/guest/xg_dom_bzimageloader.c
> > > > > > > @@ -272,8 +272,7 @@ static int _xc_try_lzma_decode(
> > > > > > >        return retval;
> > > > > > >    }
> > > > > > >    -/* 128 Mb is the minimum size (half-way) documented to work for
> > > > > > > all inputs. */
> > > > > > > -#define LZMA_BLOCK_SIZE (128*1024*1024)
> > > > > > > +#define LZMA_BLOCK_SIZE (256*1024*1024)
> > > > > > That's as arbitrary as before, now just not even with a comment at
> > > > > > least
> > > > > > hinting at it being arbitrary. Quoting from one of the LZMA API
> > > > > > headers:
> > > > > >
> > > > > >       * Decoder already supports dictionaries up to 4 GiB - 1 B (i.e.
> > > > > >       * UINT32_MAX), so increasing the maximum dictionary size of the
> > > > > >       * encoder won't cause problems for old decoders.
> > > > > >
> > > > > > IOW - what if the Linux folks decided to increase the dictionary size
> > > > > > further? I therefore wonder whether we don't need to make this more
> > > > > > dynamic, perhaps by peeking into the header to obtain the dictionary
> > > > > > size used. The one thing I'm not sure about is whether there can't be
> > > > > > multiple such headers throughout the file, and hence (in principle)
> > > > > > differing dictionary sizes.
> > > > > What is the purpose of this block size limit? From the error
> > > > > message, it
> > > > > seems to be avoiding excessive memory usage during decompression (which
> > > > > could be DoS via OOM). If that's the case, then taking the limit from
> > > > > the kernel binary itself will miss this point (especially in case of
> > > > > pygrub or similar, but there may be other cases of not-fully-trusted
> > > > > kernel binaries).
> > > > Indeed. The question then simply is: Where do we want to draw the line
> > > > between what we permit and what we reject?
> > > IMHO the most natural solution would be to use guest memory for this
> > > purpose.
> > > OTOH this probably would require a significant rework of libxenguest.
> > That was XSA-25.  There are toolstack-provided limits on kernel&initrd
> > sizes.
> Which probably can't be directly applied to dictionary size used during
> (de)compression.
My point still stands: using GUEST memory for all the decompression work
would avoid all these problems. If the guest memory isn't sufficient, a
decompression by e.g. grub wouldn't work either.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature