Re: [RFC PATCH 1/1] virtio: write back features before verify

[Cornelia Huck]

On Thu, Sep 30 2021, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:

> On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote:
>> This patch fixes a regression introduced by commit 82e89ea077b9
>> ("virtio-blk: Add validation for block size in config space") and
>> enables similar checks in verify() on big endian platforms.
>> 
>> The problem with checking multi-byte config fields in the verify
>> callback, on big endian platforms, and with a possibly transitional
>> device is the following. The verify() callback is called between
>> config->get_features() and virtio_finalize_features(). That we have a
>> device that offered F_VERSION_1 then we have the following options
>> either the device is transitional, and then it has to present the legacy
>> interface, i.e. a big endian config space until F_VERSION_1 is
>> negotiated, or we have a non-transitional device, which makes
>> F_VERSION_1 mandatory, and only implements the non-legacy interface and
>> thus presents a little endian config space. Because at this point we
>> can't know if the device is transitional or non-transitional, we can't
>> know do we need to byte swap or not.
>
> Hmm which transport does this refer to?
> Distinguishing between legacy and modern drivers is transport
> specific.  PCI presents
> legacy and modern at separate addresses so distinguishing
> between these two should be no trouble.

Hm, what about transitional devices?

> Channel i/o has versioning so same thing?

It can turn off VERSION_1, but not legacy. (I had hacked up a patchset
to potentially disable legacy some time ago, but did not have any
resources to follow up on this.)

>
>> The virtio spec explicitly states that the driver MAY read config
>> between reading and writing the features so saying that first accessing
>> the config before feature negotiation is done is not an option. The
>> specification ain't clear about setting the features multiple times
>> before FEATURES_OK, so I guess that should be fine.
>> 
>> I don't consider this patch super clean, but frankly I don't think we
>> have a ton of options. Another option that may or man not be cleaner,
>> but is also IMHO much uglier is to figure out whether the device is
>> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
>> according tho what we have figured out, hoping that the characteristics
>> of the device didn't change.
>
> I am confused here. So is the problem at the device or at the driver level?
> I suspect it's actually the host that has the issue, not
> the guest?

>From my perspective the problem is that the version of the device
remains in limbo as long as the features have not yet been finalized,
which means that the endianness of the config space remains in limbo as
well. Both device and driver might come to different conclusions.

>
>
>> Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
>> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
>> Reported-by: markver@xxxxxxxxxx
>> ---
>>  drivers/virtio/virtio.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 0a5b54034d4b..9dc3cfa17b1c 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
>>  		if (device_features & (1ULL << i))
>>  			__virtio_set_bit(dev, i);
>>  
>> +	/* Write back features before validate to know endianness */
>> +	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>> +		dev->config->finalize_features(dev);
>> +
>>  	if (drv->validate) {
>>  		err = drv->validate(dev);
>>  		if (err)
>> 
>> base-commit: 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
>> -- 
>> 2.25.1