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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Oct 09, 2024 at 01:38:32PM +0200, Jürgen Groß wrote:
> 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.

Doing that would probably require mapping guest memory to dom0 for this
purpose. And probably quite severe changes to the decompressing code
(liblzma?) to actually use that memory instead of standard heap. I don't
think it's a feasible short term fix.
Theoretically this could be made configurable (if nothing else, then via
an env variable or even build-time setting...), but honestly it feels
like an overkill.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature