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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ilh2quto.fsf@ubik.fi.intel.com>
Date:   Thu, 19 Jan 2023 19:48:35 +0200
From:   Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     mst@...hat.com, jasowang@...hat.com,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, elena.reshetova@...el.com,
        kirill.shutemov@...ux.intel.com, Andi Kleen <ak@...ux.intel.com>,
        Amit Shah <amit@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        alexander.shishkin@...ux.intel.com
Subject: Re: [PATCH v1 2/6] virtio console: Harden port adding

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

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