Re: [PATCH net] nfc: llcp: avoid userspace overflow on invalid optlen

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
nfc: llcp: avoid userspace overflow on invalid optlen

This commit attempts to fix a userspace buffer overflow in
nfc_llcp_getsockopt() by rejecting calls where the provided optlen is less
than the size of a u32.

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index f1be1e84f6653..dc74e725ddd05 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -319,6 +319,9 @@ static int nfc_llcp_getsockopt(struct socket *sock, int level, int optname,
>  	if (get_user(len, optlen))
>  		return -EFAULT;
>  
> +	if (len < sizeof(u32))
> +		return -EINVAL;

Since len is a signed int and sizeof(u32) is an unsigned size_t, does C
integer promotion cause negative lengths to bypass this check?

If a user provides an optlen of -1, it appears it would be promoted to a
large positive number, making this condition false.

It would then be explicitly cast to u32 in the min_t() call below, allowing
the code to continue and overflow the userspace buffer exactly as this
patch was attempting to prevent.

> +
>  	local = llcp_sock->local;
>  	if (!local)
>  		return -ENODEV;

While looking at this, is there a potential use-after-free regression here
if llcp_sock->local is read outside of the socket lock?

If another thread calls llcp_sock_bind() on the same socket, it might
successfully set llcp_sock->local, but then hit an error path. In that
error path, bind() would drop the reference and set llcp_sock->local
to NULL.

If the getsockopt() thread reads local here, gets the valid pointer, and
then blocks waiting for lock_sock(sk):

getsockopt() thread:
local = llcp_sock->local;
// blocks on lock_sock(sk)

bind() thread:
// hits error path
nfc_llcp_local_put(llcp_sock->local);
llcp_sock->local = NULL;
release_sock(sk);

Will the getsockopt() thread wake up, acquire the lock, and then
dereference the now-freed local pointer in the switch statement below?