Virtualization - Jan 2023 - [PATCH v1 4/6] virtio console: Harden control message handling

On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin
wrote:
> In handle_control_message(), we look at the ->event field twice, which
> gives a malicious VMM a window in which to switch it from PORT_ADD to
> PORT_REMOVE, triggering a null dereference further down the line:
How is the other VMM have full control over the full message here?
Shouldn't this all have been copied into our local memory if we are
going to be poking around in it?  Like I mentioned in my other review,
copy it all once and then parse it.  Don't try to mess with individual
fields one at a time otherwise that way lies madness...

thanks,

greg k-h

On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman
wrote:
> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> > In handle_control_message(), we look at the ->event field twice,
which
> > gives a malicious VMM a window in which to switch it from PORT_ADD to
> > PORT_REMOVE, triggering a null dereference further down the line:
> 
> How is the other VMM have full control over the full message here?
> Shouldn't this all have been copied into our local memory if we are
> going to be poking around in it?  Like I mentioned in my other review,
> copy it all once and then parse it.  Don't try to mess with individual
> fields one at a time otherwise that way lies madness...
> 
> thanks,
> 
> greg k-h
I agree and in fact, it is *already* copied since with malicious
device we generally use a bounce buffer.
Having said that, the patch is actually a cleanup, e.g. it's clearer
to byte-swap only once.
Just don't oversell it as a security thing.


-- 
MST