Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Jun 2023 14:20:55 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
Cc:     davem@davemloft.net, david@fries.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, zbr@ioremap.net,
        brauner@kernel.org, johannes@sipsolutions.net,
        ecree.xilinx@gmail.com, leon@kernel.org, keescook@chromium.org,
        socketcan@hartkopp.net, petrm@nvidia.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH v5 6/6] connector/cn_proc: Allow non-root users access
Message-ID: <20230601182055.xkcxmz5x5rc6fxzj@revolver>
References: <20230420202709.3207243-1-anjali.k.kulkarni@oracle.com>
 <20230420202709.3207243-7-anjali.k.kulkarni@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230420202709.3207243-7-anjali.k.kulkarni@oracle.com>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?G7k7gX6KxIXneUsyJVTw/4vNusHYRTXSolhy+SMycLKacxNw7zELPe+RmL+6?=
 =?us-ascii?Q?erf+1K9C6IW411LpTfK4CajN6MYG4jha8dIo32/mA4ZX32KV67QMODncWYGe?=
 =?us-ascii?Q?cYL9vWDyE0m5pMuak/05Suls2XW1TuWthFcPyjZ7lAHNMyPIUKsuG6p4ReFx?=
 =?us-ascii?Q?tvBHxRghfhvBmDRfN0Rb2iVRRi2dCz0EdB8uUDkuVYARlHvTvYd5KrgMinAS?=
 =?us-ascii?Q?GKPhi3C3xFtM6PlOsOZ0BVnnEiQkZKZnX+gfyzHAFGSbRURhCyLLOQ5exx3k?=
 =?us-ascii?Q?N3YHq+fXl9SukTlktmzJkFRECZIdRb2PsDFFAv66/BNj+Rc+0pBIpT61QhfT?=
 =?us-ascii?Q?fIC5TvEveY3Bc1q83EEMRzt1zDNeZsE5VfxQKY1UAizme1tZ/K1OZj+CxPn+?=
 =?us-ascii?Q?GPRsoP3fW0lvNT7T0ZgXe9Cqp/+yhCsulbthB3oLPdTBqgJndLxjP+CHWpM+?=
 =?us-ascii?Q?wFLi+qxld+vBMFzsZ5wt0KqeNlfWWgwbhpqv0g8VazfQYYNT2zQDNXr96WdV?=
 =?us-ascii?Q?eTL25NJ6G4N3gi8WQyXpHHrZ+VtDXQqLvTfZF1w7s2+ms1+lshbPtkY+wNqj?=
 =?us-ascii?Q?Y8w7qLKavM/+a7pUd7GupkRT2M17yxQp8Em/4TSO50u4RNKSkxooX/kamwWU?=
 =?us-ascii?Q?5prZvvzCXfgJkVhW4WkPJ6JhcrtH6pWk5x7UJoRbSfhhtrpElM247ywVo3qP?=
 =?us-ascii?Q?dZM9JKy0jyx4KYCnkGn4L03t6N/aap/w84o4e+FpcTrom1A1fmIJ7cIRN8NZ?=
 =?us-ascii?Q?drcoVuEeJfDqBTGAgGpxS/IOHZm9ydnQagVdBO14zldRL5I+qTzed+KnOYjh?=
 =?us-ascii?Q?Z9ISXdhY99Qp57piDupUGdpQenwgpFETGBE4RoMh9xZuC77VROxW3lo10lqo?=
 =?us-ascii?Q?3EhllRblquhzr6v0WoTU6UaS7pNCxXzTWb+RgpvLM9exSoOc25HFn0yUyIoP?=
 =?us-ascii?Q?I5yFMcnVMcv+E4hxtDsyefZeqeaN1TLsbNQib9bjvSHi2OlJOfWaMyERu27R?=
 =?us-ascii?Q?Jtzn4Q9L0ylJsemNuLtDPD2nmGPpnr9jashDqQMQI5/2Aou5es2A+FxZ7dvn?=
 =?us-ascii?Q?RXSoZXCZYlXhEH0Oh3948Yz7IrXRffmTajhiRoCJ5q6IAt79BdFUIpV5+sat?=
 =?us-ascii?Q?lp22f5+JzmFG+UlOU3rJRz+sCTiSE/X+t9PDxYvJHWTqrc1v1C3bLqtcFoJy?=
 =?us-ascii?Q?rV+O0/2sfSRaY0DPIpvLfg33brgqng/Cmirr78mFxr+XRgX+SQoMfEvnyVMx?=
 =?us-ascii?Q?U76Km3FmMfaXHF4FXYg9piSnXKHn/T7qKVAfkFlVOMxXGrKJobGBEc4q44vl?=
 =?us-ascii?Q?QtOyjLh6j8e9C6XCTIA/b4NJLO2jSuI8L7RV4ZUYiAaaJ1AiVNdMuTIpVX+Y?=
 =?us-ascii?Q?gg7cQt0Jj8eg01l5EsKgFRw/2i9imG49Kbwb3Kay7NVkgoXC9SGdEiP1cKap?=
 =?us-ascii?Q?74JAgXZ2Kr0b/ezNSr3gBAMfznqCUzBgs78ARu/VTIqQoQvecnl2lKqNeRye?=
 =?us-ascii?Q?bcJP5iPah+34egmEzYb0HiQwcXzpW8JXmv+FORUS/g9PbYlaJrzBHtWptF+U?=
 =?us-ascii?Q?MTrYLeaHQLTyB04PTR+8ZiDMY6hoKM15YbVRsOXoPcmuvQGbWAWgtayNazvO?=
 =?us-ascii?Q?eg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?8ZTU0SBzN3yKRw9Kjfa7YRvqdh1n79L4d12TLBM1fGOPgAzeXzPOk38pO8ge?=
 =?us-ascii?Q?IfWPTGcpjrkCwkHIHBTewtUw7sihW6h2g/Jq3gpx6Crl5tCUBcST+c55QBNJ?=
 =?us-ascii?Q?JEq5wJAkW3HUcdNfGau7zQu5cXK/Z/6BpH6Dmf2iwLcFb3VmW502sGLfA7mx?=
 =?us-ascii?Q?JdfEE7QikQpcZKfRITTYog6/WWkEKTL78jiGyCygM5Sim9Gn/xycU4Jer77R?=
 =?us-ascii?Q?rc+qaCUbPl4UX15ooFlZuSMjLtyiJj6adzOBid1wvWER5/lUtyIdHdx/FUH2?=
 =?us-ascii?Q?rE2CR2lJn+Dk394EfpCM7mcauhEZc9kklwb1cR3C8dKglLZi24Bnv8B4ofWr?=
 =?us-ascii?Q?xY6scl7YMPMpe4X+XBC92oCO+EL88NyUCHzURpujp0Uo/ZH1PWwFM/+b10Ic?=
 =?us-ascii?Q?3L9WQKHg+zcUSrD+WFvKu8ItyA9gkl6c+ZB3jKRGN1+4h3yAf+DRm71NhSDX?=
 =?us-ascii?Q?6NBaX1IdhV3e/Ost/D6ZzwssYAGQQbbGo2H1ymO4Ut/xzur5ssMZmq7UQdBt?=
 =?us-ascii?Q?1eGhuVly6wKAFdCtBb66I1do4cCqdLFimOTAy+ibG/ZmU4yyTwysW2q/3CQf?=
 =?us-ascii?Q?KOkanGcAbURgMAUByyrRlcKRt3OrlpyzHZ2Ms9rT+fcc3sd7Fl66X7U3jg8C?=
 =?us-ascii?Q?vVb5uZdZuCHl341gJv/e8bq8O/bemTdmX/I7kD9nq/hw0Wd4Ys6p6AHFEPar?=
 =?us-ascii?Q?ltH/7Wx31jtvWia2yX5/UGWeV4Ko/TJP4w7qfOzkWw6Iy0SfCjO/w726Nen9?=
 =?us-ascii?Q?zz8La2icbg7+8IGeKKvAtZc2hfKCTqEegeKQ7SmCZ4Txqblcifif0AACStxz?=
 =?us-ascii?Q?m+r6VATQNZeMT0cpuNi4fgZBKytsZwrecxOAGJrfJFOEXPeukILCA2ZjtGYr?=
 =?us-ascii?Q?r3KO6DCeLgw945ATqCiscVb4f92L200eiI8bl8cbH8Iu020Ez4DQFGK1V+Qz?=
 =?us-ascii?Q?Q+45gyrDeik9ji3pQx3bOhrPOZawy9YvwwClDBG+racfe2FYu1p3bgYwH2VU?=
 =?us-ascii?Q?DRDc5ZTPr8NA8C1RMdOjW8RJBK3abcCFl77MpFr9neei1Vf77cBzwA4rJ1ZL?=
 =?us-ascii?Q?230jNrbZq+OLwsnALtP1zeTmaSRomy/qIF9IL0inObLvfhpP0fo=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49b62411-c30d-4fd4-3fec-08db62ccf297
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jun 2023 18:20:58.7378
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: g5KuIBrcRpHTrIAW0XDU8MYwD/ClF8LbUUtgLGZT4MBbngP1J8tol947cIn29AGoTEmYh56ZoetgwvC2uQKlyw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7107
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-06-01_08,2023-05-31_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxscore=0 mlxlogscore=999
 suspectscore=0 malwarescore=0 adultscore=0 phishscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000
 definitions=main-2306010158
X-Proofpoint-GUID: 2u6PJyJ5oHDLEOVEnkJISyD7YCRqCr1_
X-Proofpoint-ORIG-GUID: 2u6PJyJ5oHDLEOVEnkJISyD7YCRqCr1_
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Anjali Kulkarni <anjali.k.kulkarni@oracle.com> [691231 23:00]:
> There were a couple of reasons for not allowing non-root users access
> initially  - one is there was some point no proper receive buffer
> management in place for netlink multicast. But that should be long
> fixed. See link below for more context.
> 
> Second is that some of the messages may contain data that is root only. But
> this should be handled with a finer granularity, which is being done at the
> protocol layer.  The only problematic protocols are nf_queue and the
> firewall netlink. Hence, this restriction for non-root access was relaxed
> for NETLINK_ROUTE initially:
> https://lore.kernel.org/all/20020612013101.A22399@wotan.suse.de/
> 
> This restriction has also been removed for following protocols:
> NETLINK_KOBJECT_UEVENT, NETLINK_AUDIT, NETLINK_SOCK_DIAG,
> NETLINK_GENERIC, NETLINK_SELINUX.
> 
> Since process connector messages are not sensitive (process fork, exit
> notifications etc.), and anyone can read /proc data, we can allow non-root
> access here. However, since process event notification is not the only
> consumer of NETLINK_CONNECTOR, we can make this change even more
> fine grained than the protocol level, by checking for multicast group
> within the protocol.
> 
> Allow non-root access for NETLINK_CONNECTOR via NL_CFG_F_NONROOT_RECV
> but add new bind function cn_bind(), which allows non-root access only
> for CN_IDX_PROC multicast group.
> 
> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
> ---
>  drivers/connector/cn_proc.c   |  7 -------
>  drivers/connector/connector.c | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 35bec1fd7ee0..046a8c1d8577 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -408,12 +408,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>  	    !task_is_in_init_pid_ns(current))
>  		return;
>  
> -	/* Can only change if privileged. */
> -	if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) {
> -		err = EPERM;
> -		goto out;
> -	}
> -
>  	if (msg->len == sizeof(*pinput)) {
>  		pinput = (struct proc_input *)msg->data;
>  		mc_op = pinput->mcast_op;
> @@ -460,7 +454,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>  		break;
>  	}
>  
> -out:
>  	cn_proc_ack(err, msg->seq, msg->ack);
>  }
>  
> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
> index d1179df2b0ba..193d3056de64 100644
> --- a/drivers/connector/connector.c
> +++ b/drivers/connector/connector.c
> @@ -166,6 +166,18 @@ static int cn_call_callback(struct sk_buff *skb)
>  	return err;
>  }
>  

Should there be a comment here about non-root access?

> +static int cn_bind(struct net *net, int group)
> +{
> +	unsigned long groups = 0;
> +	groups = (unsigned long) group;

I don't understand why you have this cast on a second line or why you
zero groups before the assignment?  It would all fit on one line.

> +
> +	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return 0;

New line here please

> +	if (test_bit(CN_IDX_PROC - 1, &groups))
> +		return 0;

New line here please

> +	return -EPERM;
> +}
> +
>  static void cn_release(struct sock *sk, unsigned long *groups)
>  {
>  	if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
> @@ -261,6 +273,8 @@ static int cn_init(void)
>  	struct netlink_kernel_cfg cfg = {
>  		.groups	= CN_NETLINK_USERS + 0xf,
>  		.input	= cn_rx_skb,
> +		.flags  = NL_CFG_F_NONROOT_RECV,
> +		.bind   = cn_bind,
>  		.release = cn_release,
>  	};
>  
> -- 
> 2.40.0
>