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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>)?

Thanks, Roger.