netdev - Re: [PATCH net-next v2 3/3] genetlink: fit NLMSG

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]

Date: Fri, 15 Mar 2024 12:48:08 +0100
From: Stefano Brivio <sbrivio@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, jiri@...nulli.us, idosch@...sch.org,
 johannes@...solutions.net, fw@...len.de, pablo@...filter.org, Martin Pitt
 <mpitt@...hat.com>, Paul Holzinger <pholzing@...hat.com>, David Gibson
 <david@...son.dropbear.id.au>
Subject: Re: [PATCH net-next v2 3/3] genetlink: fit NLMSG_DONE into same
 read() as families

Hi,

On Sat,  2 Mar 2024 21:24:08 -0800
Jakub Kicinski <kuba@...nel.org> wrote:

> Make sure ctrl_fill_info() returns sensible error codes and
> propagate them out to netlink core. Let netlink core decide
> when to return skb->len and when to treat the exit as an
> error. Netlink core does better job at it, if we always
> return skb->len the core doesn't know when we're done
> dumping and NLMSG_DONE ends up in a separate read().

While this change is obviously correct, it breaks... well, broken
applications that _wrongly_ rely on the fact that NLMSG_DONE is
delivered in a separate datagram.

This was the (embarrassing) case for passt(1), which I just fixed:
  https://archives.passt.top/passt-dev/20240315112432.382212-1-sbrivio@redhat.com/

but the "separate" NLMSG_DONE is such an established behaviour,
I think, that this might raise a more general concern.

From my perspective, I'm just happy that this change revealed the
issue, but I wanted to report this anyway in case somebody has
similar possible breakages in mind.

> Reviewed-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: jiri@...nulli.us
> ---
>  net/netlink/genetlink.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 50ec599a5cff..3b7666944b11 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1232,7 +1232,7 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq,
>  
>  	hdr = genlmsg_put(skb, portid, seq, &genl_ctrl, flags, cmd);
>  	if (hdr == NULL)
> -		return -1;
> +		return -EMSGSIZE;
>  
>  	if (nla_put_string(skb, CTRL_ATTR_FAMILY_NAME, family->name) ||
>  	    nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, family->id) ||
> @@ -1355,6 +1355,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
>  	struct net *net = sock_net(skb->sk);
>  	int fams_to_skip = cb->args[0];
>  	unsigned int id;
> +	int err = 0;
>  
>  	idr_for_each_entry(&genl_fam_idr, rt, id) {
>  		if (!rt->netnsok && !net_eq(net, &init_net))
> @@ -1363,16 +1364,17 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (n++ < fams_to_skip)
>  			continue;
>  
> -		if (ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
> -				   cb->nlh->nlmsg_seq, NLM_F_MULTI,
> -				   skb, CTRL_CMD_NEWFAMILY) < 0) {
> +		err = ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
> +				     cb->nlh->nlmsg_seq, NLM_F_MULTI,
> +				     skb, CTRL_CMD_NEWFAMILY);
> +		if (err) {
>  			n--;
>  			break;
>  		}
>  	}
>  
>  	cb->args[0] = n;
> -	return skb->len;
> +	return err;
>  }
>  
>  static struct sk_buff *ctrl_build_family_msg(const struct genl_family *family,

-- 
Stefano