From: 'Simon Horman' <[email protected]> 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?
