Re: [PATCH v1 2/6] virtio console: Harden port adding

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 07:48:35PM +0200, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> 
> > On Thu, Jan 19, 2023 at 03:57:17PM +0200, Alexander Shishkin wrote:
> >> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> >> 
> >> The ADD_PORT operation reads and sanity checks the port id multiple
> >> times from the untrusted host. This is not safe because a malicious
> >> host could change it between reads.
> >> 
> >> Read the port id only once and cache it for subsequent uses.
> >> 
> >> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> >> Cc: Amit Shah <amit@xxxxxxxxxx>
> >> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/char/virtio_console.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >> index f4fd5fe7cd3a..6599c2956ba4 100644
> >> --- a/drivers/char/virtio_console.c
> >> +++ b/drivers/char/virtio_console.c
> >> @@ -1563,10 +1563,13 @@ static void handle_control_message(struct virtio_device *vdev,
> >>  	struct port *port;
> >>  	size_t name_size;
> >>  	int err;
> >> +	unsigned id;
> >>  
> >>  	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
> >>  
> >> -	port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id));
> >> +	/* Make sure the host cannot change id under us */
> >> +	id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
> >
> > Why READ_ONCE()?
> >
> > And how can it change under us?  Is the message still under control of
> > the "host"?  If so, that feels wrong as this is all in kernel memory,
> > not userspace memory right?
> >
> > If you are dealing with memory from a different process that you do not
> > trust, then you need to copy EVERYTHING at once.  Don't piece-meal copy
> > bits and bobs in all different places please.  Do it once and then parse
> > the local structure properly.
> 
> This is the device memory or the VM host memory, not userspace or
> another process. And it can change under us willy-nilly.

Then you need to copy it out once, and then only deal with the local
copy.  Otherwise you have an incomplete snapshot.

> The thing is, we only need to cache two things to correctly process the
> request. Copying everything, on the other hand, would involve the entire
> buffer, not just the *cpkt, but also stuff that follows, which also
> differs between different event types. And we also don't care if the
> rest of it changes under us.

That feels broken if you do not "trust" that other side.  And what
prevents the buffer from changing after you validated the other part?

For virtio, I thought you always implied that you did trust the other
side, when has that changed?  Where was that new security model for the
kernel discussed?

Are you sure this is even viable?  What is the threat model you are
attempting to add to the driver here?

> > Otherwise this is going to be impossible to actually maintain over
> > time...
> 
> An 'id' can't possibly be worse to maintain than multiple instances of
> 'virtio32_to_cpu(vdev, cpkt->id)' sprinkled around the code.

Again, copy what you want out and then act on that.  If it can change
under you, and you do not trust it, then you have to work only on a
snapshot that you have verified.

thanks,

greg k-h