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

[Alexander Shishkin]

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.

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.

> 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.

Thanks,
--
Alex