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

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

On Mon, Dec 16, 2024 at 09:35:37AM +0100, Roger Pau Monné wrote:
> Adding Oleksii, as this IMO wants to be a blocker for 4.20.
> 
> On Wed, Oct 09, 2024 at 03:03:28PM +0200, Marek Marczykowski-Górecki wrote:
> > 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.
> 
> As a compromise that could likely be done in time for the release,
> would it be feasible to fetch the dictionary size from the header and
> cap it at certain boundary using max(<header val>, <boundary>)?

Isn't the current constant more or less that already? It's named
LZMA_BLOCK_SIZE, but the lzma_stream_decoder() argument it's used for is
"memlimit", described as "Memory usage limit as bytes".

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

Attachment: signature.asc
Description: PGP signature