netdev - Re: [PATCH net] net: Check for oversized requests in sock

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] [day] [month] [year] [list]

Message-ID: <2251fed2-418e-417c-ba27-b2a177f26384@rbox.co>
Date: Mon, 16 Dec 2024 22:42:01 +0100
From: Michal Luczaj <mhal@...x.co>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 herbert@...dor.apana.org.au
Subject: Re: [PATCH net] net: Check for oversized requests in sock_kmalloc()

+cc Herbert. Allocator warning due to sock_kmalloc() in
crypto/af_alg.c:alg_setkey(). Please see the cover letter for a repro:
https://lore.kernel.org/netdev/20241216-sock-kmalloc-warn-v1-1-9cb7fdee5b32@rbox.co/

More comments below:

On 12/16/24 13:43, Eric Dumazet wrote:
> On Mon, Dec 16, 2024 at 12:51 PM Michal Luczaj <mhal@...x.co> wrote:
>>
>> Allocator explicitly rejects requests of order > MAX_PAGE_ORDER, triggering
>> a WARN_ON_ONCE_GFP().
>>
>> Put a size limit in sock_kmalloc().
>>
>> WARNING: CPU: 6 PID: 1676 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x32e/0x3a0
>> Call Trace:
>>  ___kmalloc_large_node+0x71/0xf0
>>  __kmalloc_large_node_noprof+0x1b/0xf0
>>  __kmalloc_noprof+0x436/0x560
>>  sock_kmalloc+0x44/0x60
>>  ____sys_sendmsg+0x208/0x3a0
>>  ___sys_sendmsg+0x84/0xd0
>>  __sys_sendmsg+0x56/0xa0
>>  do_syscall_64+0x93/0x180
>>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> I would rather change ____sys_sendmsg() to use something different
> than sock_kmalloc().
> 
> This would avoid false sharing (on sk->sk_omem_alloc) for a short-lived buffer,
> and could help UDP senders sharing a socket...
> 
> sock_kmalloc() was really meant for small and long lived allocations
> (corroborated by net.core.optmem_max small default value)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 9a117248f18f13d574d099c80128986c744fa97f..c23d8e20c5c626c54b9a04a416b82f696fa2310c
> 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2552,7 +2552,8 @@ static int ____sys_sendmsg(struct socket *sock,
> struct msghdr *msg_sys,
>                 BUILD_BUG_ON(sizeof(struct cmsghdr) !=
>                              CMSG_ALIGN(sizeof(struct cmsghdr)));
>                 if (ctl_len > sizeof(ctl)) {
> -                       ctl_buf = sock_kmalloc(sock->sk, ctl_len, GFP_KERNEL);
> +                       ctl_buf = kvmalloc(ctl_len, GFP_KERNEL_ACCOUNT |
> +                                                   __GFP_NOWARN);
>                         if (ctl_buf == NULL)
>                                 goto out;
>                 }
> @@ -2594,7 +2595,7 @@ static int ____sys_sendmsg(struct socket *sock,
> struct msghdr *msg_sys,
> 
>  out_freectl:
>         if (ctl_buf != ctl)
> -               sock_kfree_s(sock->sk, ctl_buf, ctl_len);
> +               kvfree(ctl_buf);
>  out:
>         return err;
>  }

Got it. I guess the same treatment for cmsghdr_from_user_compat_to_kern()?

Similar problem is with compat_ip_set_mcast_msfilter() and
compat_ipv6_set_mcast_msfilter(), direct kmalloc() this time.