Re: [Qemu-block] [PATCH v2 0/7] Refactor DMG driver to have chunk size independence

[Stefan Hajnoczi]

On Thu, Apr 27, 2017 at 01:36:30PM +0530, Ashijeet Acharya wrote:
> This series helps to provide chunk size independence for DMG driver to prevent
> denial-of-service in cases where untrusted files are being accessed by the 
> user.

The core of the chunk size dependence problem are these lines:

    s->compressed_chunk = qemu_try_blockalign(bs->file->bs,
                                              ds.max_compressed_size + 1);
    s->uncompressed_chunk = qemu_try_blockalign(bs->file->bs,
                                                512 * ds.max_sectors_per_chunk);

The refactoring needs to eliminate these buffers because their size is
controlled by the untrusted input file.

After applying your patches these lines remain unchanged and we still
cannot use input files that have a 250 MB chunk size, for example.  So
I'm not sure how this series is supposed to work.

Here is the approach I would take:

In order to achieve this dmg_read_chunk() needs to be scrapped.  It is
designed to read a full chunk.  The new model does not read full chunks
anymore.

Uncompressed reads or zeroes should operate directly on qiov, not
s->uncompressed_chunk.  This code will be dropped:

    data = s->uncompressed_chunk + sector_offset_in_chunk * 512;
    qemu_iovec_from_buf(qiov, i * 512, data, 512);

Compressed reads still buffers.  I suggest the following buffers:

1. compressed_buf - compressed data is read into this buffer from file
2. uncompressed_buf - a place to discard decompressed data while
                      simulating a seek operation

Data is read from compressed chunks by reading a reasonable amount
(64k?) into compressed_buf.  If the user wishes to read at an offset
into this chunk then a loop decompresses data we are seeking over into
uncompressed_buf (and refills compressed_buf if it becomes empty) until
the desired offset is reached.  Then decompression can continue
directly into the user's qiov and uncompressed_buf isn't used to
decompress the data requested by the user.

Sequential compressed reads can be optimized by keeping the compression
state across read calls.  That means the zlib/bz2 state plus
compressed_buf and the current offset.  That way we don't need to
re-seek into the current compressed chunk to handle sequential reads.

signature.asc
Description: PGP signature