On 2/9/21 6:11 PM, Eric Blake wrote:
> On 2/9/21 11:01 AM, Philippe Mathieu-Daudé wrote:
>> The null-co driver is meant for (performance) testing.
>> By default, read operation does nothing, the provided buffer
>> is not filled with zero values and its content is unchanged.
>>
>> This can confuse security experts. For example, using the default
>> null-co driver, buf[] is uninitialized, the blk_pread() call
>> succeeds and we then access uninitialized memory:
>>
>> static int guess_disk_lchs(BlockBackend *blk,
>> int *pcylinders, int *pheads,
>> int *psectors)
>> {
>> uint8_t buf[BDRV_SECTOR_SIZE];
>> ...
>>
>> if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
>> return -1;
>> }
>> /* test msdos magic */
>> if (buf[510] != 0x55 || buf[511] != 0xaa) {
>> return -1;
>> }
>>
>> We could audit all the uninitialized buffers and the
>> bdrv_co_preadv() handlers, but it is simpler to change the
>> default of this testing driver. Performance tests will have
>> to adapt and use 'null-co,read-zeroes=on'.
>
> Wouldn't this rather be read-zeroes=off when doing performance testing?
Oops, yes ;)
>
>>
>> Suggested-by: Max Reitz <[email protected]>
>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
>> ---
>> RFC maybe a stricter approach is required?
>
> Since the null driver is only for testing in the first place, opting in
> to speed over security seems like a reasonable tradeoff. But I consider
> the patch incomplete without an audit of the iotests that will want to
> use explicit read-zeroes=off.
Correct. I don't know about each iotest but I can send a patch with
explicit option, so review would be trivial.
Thanks,
Phil.
