netdev - Re: [PATCH net 1/2] netlink: fix false positive warning in extack during dumps

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: <20241115140007.GR1062410@kernel.org>
Date: Fri, 15 Nov 2024 14:00:07 +0000
From: Simon Horman <horms@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com,
	syzbot+d4373fa8042c06cefa84@...kaller.appspotmail.com,
	dsahern@...nel.org
Subject: Re: [PATCH net 1/2] netlink: fix false positive warning in extack
 during dumps

On Thu, Nov 14, 2024 at 04:31:49PM -0800, Jakub Kicinski wrote:
> Commit under fixes extended extack reporting to dumps.
> It works under normal conditions, because extack errors are
> usually reported during ->start() or the first ->dump(),
> it's quite rare that the dump starts okay but fails later.
> If the dump does fail later, however, the input skb will
> already have the initiating message pulled, so checking
> if bad attr falls within skb->data will fail.
> 
> Switch the check to using nlh, which is always valid.
> 
> syzbot found a way to hit that scenario by filling up
> the receive queue. In this case we initiate a dump
> but don't call ->dump() until there is read space for
> an skb.
> 
> RIP: 0010:netlink_ack_tlv_fill+0x1a8/0x560 net/netlink/af_netlink.c:2209
> Call Trace:
>  <TASK>
>  netlink_dump_done+0x513/0x970 net/netlink/af_netlink.c:2250
>  netlink_dump+0x91f/0xe10 net/netlink/af_netlink.c:2351
>  netlink_recvmsg+0x6bb/0x11d0 net/netlink/af_netlink.c:1983
>  sock_recvmsg_nosec net/socket.c:1051 [inline]
>  sock_recvmsg+0x22f/0x280 net/socket.c:1073
>  __sys_recvfrom+0x246/0x3d0 net/socket.c:2267
>  __do_sys_recvfrom net/socket.c:2285 [inline]
>  __se_sys_recvfrom net/socket.c:2281 [inline]
>  __x64_sys_recvfrom+0xde/0x100 net/socket.c:2281
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>  RIP: 0033:0x7ff37dd17a79
> 
> Reported-by: syzbot+d4373fa8042c06cefa84@...kaller.appspotmail.com
> Fixes: 8af4f60472fc ("netlink: support all extack types in dumps")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: dsahern@...nel.org
> ---
>  include/net/netlink.h    | 13 +++++++++++++
>  net/netlink/af_netlink.c | 13 +++++--------
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index db6af207287c..efd906aff22f 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -659,6 +659,19 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
>  	return (struct nlmsghdr *) ((unsigned char *) nlh + totlen);
>  }
>  
> +/**
> + * nlmsg_addr_in_payload - address points to a byte within the message payload
> + * @nlh: netlink message header

nit: something about @addr should go here.

> + *
> + * Returns: true if address is within the payload of the message
> + */
> +static inline bool nlmsg_addr_in_payload(const struct nlmsghdr *nlh,
> +					 const void *addr)
> +{
> +	return addr >= nlmsg_data(nlh) &&
> +		addr - (const void *) nlh < nlh->nlmsg_len;
> +}
> +
>  /**
>   * nla_parse - Parse a stream of attributes into a tb buffer
>   * @tb: destination array with maxtype+1 elements

Otherwise, LGTM.