On Thu, Sep 30 2021, Halil Pasic <pasic@xxxxxxxxxxxxx> 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. > > 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. > > 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); This really looks like a mess :( We end up calling ->finalize_features twice: once before ->validate, and once after, that time with the complete song and dance. The first time, we operate on one feature set; after validation, we operate on another, and there might be interdependencies between the two (like a that a bit is cleared because of another bit, which would not happen if validate had a chance to clear that bit before). I'm not sure whether that is even a problem in the spec: while the driver may read the config before finally accepting features, it does not really make sense to do so before a feature bit as basic as VERSION_1 which determines the endianness has been negotiated. For VERSION_1, we can probably go ahead and just assume that we will accept it if offered, but what about other (future) bits? > + > if (drv->validate) { > err = drv->validate(dev); > if (err) _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization