netdev - Re: [PATCH v5 1/6] netlink: Reverse the patch which removed filtering

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]
Date: Thu, 1 Jun 2023 20:24:52 +0000
From: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
To: Liam Howlett <liam.howlett@...cle.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "david@...es.net"
	<david@...es.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "zbr@...emap.net"
	<zbr@...emap.net>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        "leon@...nel.org"
	<leon@...nel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "socketcan@...tkopp.net" <socketcan@...tkopp.net>,
        "petrm@...dia.com"
	<petrm@...dia.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [PATCH v5 1/6] netlink: Reverse the patch which removed filtering



> On Jun 1, 2023, at 10:56 AM, Liam Howlett <liam.howlett@...cle.com> wrote:
> 
> * Anjali Kulkarni <anjali.k.kulkarni@...cle.com> [691231 23:00]:
>> To use filtering at the connector & cn_proc layers, we need to enable
>> filtering in the netlink layer. This reverses the patch which removed
>> netlink filtering.
>> 
>> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
>> ---
>> include/linux/netlink.h | 5 +++++
>> net/netlink/af_netlink.c | 25 +++++++++++++++++++++++--
>> 2 files changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>> index c43ac7690eca..866bbc5a4c8d 100644
>> --- a/include/linux/netlink.h
>> +++ b/include/linux/netlink.h
>> @@ -206,6 +206,11 @@ bool netlink_strict_get_check(struct sk_buff *skb);
>> int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
>> int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid,
>> 		 __u32 group, gfp_t allocation);
>> +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb,
>> +			 __u32 portid, __u32 group, gfp_t allocation,
>> +			 int (*filter)(struct sock *dsk,
>> +					 struct sk_buff *skb, void *data),
>> +			 void *filter_data);
> 
> Nit, just a personal preference that if you indent with two tabs for
> function definitions, then it is clear where the code starts and you
> have more room for larger argument lists here. It also helps when
> changing the return type as you don't have to redo all the spacing.

Thanks so much - will update my code will all your review comments plus those of Jakub & Andrew, and send it out in the next revision.

Anjali

> 
>> int netlink_set_err(struct sock *ssk, __u32 portid, __u32 group, int code);
>> int netlink_register_notifier(struct notifier_block *nb);
>> int netlink_unregister_notifier(struct notifier_block *nb);
>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>> index c64277659753..003c7e6ec9be 100644
>> --- a/net/netlink/af_netlink.c
>> +++ b/net/netlink/af_netlink.c
>> @@ -1432,6 +1432,8 @@ struct netlink_broadcast_data {
>> 	int delivered;
>> 	gfp_t allocation;
>> 	struct sk_buff *skb, *skb2;
>> +	int (*tx_filter)(struct sock *dsk, struct sk_buff *skb, void *data);
>> +	void *tx_data;
>> };
>> 
>> static void do_one_broadcast(struct sock *sk,
>> @@ -1485,6 +1487,11 @@ static void do_one_broadcast(struct sock *sk,
>> 			p->delivery_failure = 1;
>> 		goto out;
>> 	}
> 
> new line here please.
> 
>> +	if (p->tx_filter && p->tx_filter(sk, p->skb2, p->tx_data)) {
>> +		kfree_skb(p->skb2);
>> +		p->skb2 = NULL;
>> +		goto out;
>> +	}
> 
> new line here please.
> 
> Since there are now two times that the same steps are being used for
> unrolling (yours and below). It might be better to make a new goto
> label after the successful one?
> 
>> 	if (sk_filter(sk, p->skb2)) {
>> 		kfree_skb(p->skb2);
>> 		p->skb2 = NULL;
>> @@ -1507,8 +1514,12 @@ static void do_one_broadcast(struct sock *sk,
>> 	sock_put(sk);
>> }
>> 
>> -int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid,
>> -		 u32 group, gfp_t allocation)
>> +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb,
>> +			 u32 portid,
>> +			 u32 group, gfp_t allocation,
>> +			 int (*filter)(struct sock *dsk,
>> +					 struct sk_buff *skb, void *data),
>> +			 void *filter_data)
> 
> Same comment here about the two tab indent.
> 
>> {
>> 	struct net *net = sock_net(ssk);
>> 	struct netlink_broadcast_data info;
>> @@ -1527,6 +1538,8 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid,
>> 	info.allocation = allocation;
>> 	info.skb = skb;
>> 	info.skb2 = NULL;
>> +	info.tx_filter = filter;
>> +	info.tx_data = filter_data;
>> 
>> 	/* While we sleep in clone, do not allow to change socket list */
>> 
>> @@ -1552,6 +1565,14 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid,
>> 	}
>> 	return -ESRCH;
>> }
>> +EXPORT_SYMBOL(netlink_broadcast_filtered);
>> +
>> +int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid,
>> +		 u32 group, gfp_t allocation)
>> +{
>> +	return netlink_broadcast_filtered(ssk, skb, portid, group, allocation,
>> +					 NULL, NULL);
>> +}
>> EXPORT_SYMBOL(netlink_broadcast);
>> 
>> struct netlink_set_err_data {
>> -- 
>> 2.40.0