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

Alexander Shishkin
2023-01-19T17:48:52Z

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Jan 19, 2023 at 03:57:17PM +0200, Alexander Shishkin wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> 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@linux.intel.com>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Amit Shah <amit@kernel.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>> 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