Messages in this thread | | | From | Jason Wang <> | Date | Tue, 20 Dec 2022 14:44:16 +0800 | Subject | Re: [PATCH 0/4] Fix probe failed when modprobe modules |
| |
On Mon, Dec 19, 2022 at 6:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 29, 2022 at 11:37:09AM +0800, Jason Wang wrote: > > > > > > > > > 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: > > > 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 >
| |