Re: Security issue in linux/drivers/firewire/nosy.c

[Takashi S. <o-t...@sa...>]

Hi,

On Wed, Apr 17, 2024 at 11:46:47AM -0400, Thanassis Avgerinos wrote:
> Hi Takashi,
> 
> Apologies for the delay, and thank you for the super fast response! Please
> find attached a possible patch for this issue. It should be
> self-explanatory but you may have a better suggestion on how to handle it.
> Please let me know and I'm happy to make changes as needed.
> 
> After this is patched, would it be possible to issue a CVE for it? Would
> you be the best person to help me with this?
> 
> Best,
> Thanassis

Thanks for the patch to fix the TODO, however would I ask you to send it
again with your `Sign-off-by:` tag? It is mandatory in usual kernel
development process, and I can not apply any modification about it, with
the respect to copyright, sorry.

I have a nitpick about the return value at the case that the user
process provides the insufficient size of buffer against the data stored
in kernel space. In the case, the typical UNIX-like kernel returns zero
instead of -EINVAL. We should follow to the convention if we have no
specific reason, in my opinion.

> From f7ee97fabe1519225ba30fd9454344d0a75f4d94 Mon Sep 17 00:00:00 2001
> From: Thanassis Avgerinos <tha...@gm...>
> Date: Wed, 17 Apr 2024 11:30:02 -0400
> Subject: [PATCH] firewire: nosy: ensure user_length is taken into account when
>  fetching packet contents
> 
> Ensure that packet_buffer_get respects the user_length provided. If
> the length of the head packet exceeds the user_length, packet_buffer_get
> will now return -EINVAL to signify to the user that a larger data
> buffer is required. Helps prevent user space overflows.
> ---
>  drivers/firewire/nosy.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c
> index b0d671db178a..b1dccf3ba8f9 100644
> --- a/drivers/firewire/nosy.c
> +++ b/drivers/firewire/nosy.c
> @@ -148,10 +148,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length)
>  	if (atomic_read(&buffer->size) == 0)
>  		return -ENODEV;
>  
> -	/* FIXME: Check length <= user_length. */
> +	length = buffer->head->length;
> +
> +	if (length > user_length)
> +		return -EINVAL;
>  
>  	end = buffer->data + buffer->capacity;
> -	length = buffer->head->length;
>  
>  	if (&buffer->head->data[length] < end) {
>  		if (copy_to_user(data, buffer->head->data, length))
> -- 
> 2.23.0


Thanks

Takashi Sakamoto