On Mon, Dec 19, 2022 at 6:15 PM Michael S. Tsirkin [off-list ref] wrote:

On Tue, Nov 29, 2022 at 11:37:09AM +0800, Jason Wang wrote:

quoted

quoted

Quite a lot of core work here. Jason are you still looking into
hardening?

Yes, last time we've discussed a solution that depends on the first
kick to enable the interrupt handler. But after some thought, it seems
risky since there's no guarantee that the device work in this way.

One example is the current vhost_net, it doesn't wait for the kick to
process the rx packets. Any more thought on this?

Thanks

Specifically virtio net is careful to call virtio_device_ready
under rtnl lock so buffers are only added after DRIVER_OK.

Right but it only got fixed this year after some code audit.

However we do not need to tie this to kick, this is what I wrote:

quoted

BTW Jason, I had the idea to disable callbacks until driver uses the
virtio core for the first time (e.g. by calling virtqueue_add* family of
APIs). Less aggressive than your ideas but I feel it will add security
to the init path at least.

So not necessarily kick, we can make adding buffers allow the
interrupt.

Some questions:

1) It introduces a code defined behaviour other than depending on the
spec defined behavior like DRIVER_OK, this will lead extra complexity
in auditing
2) there's no guarantee that the interrupt handler is ready before
virtqueue_add(), or it requires barriers before virtqueue_add() to
make sure the handler is commit

So it looks to me the virtio_device_ready() should be still the
correct way to go:

1) it depends on spec defined behaviour like DRIVER_OK, and it then
can comply with possible future security requirement of drivers
defined in the spec
2) choose to use a new boolean instead of reusing vq->broken
3) enable the harden in driver one by one

Does it make sense?

Thanks

--
MST