Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Christoph Lameter]

Sorry slight mistake in the original patch. V2 follows



Subject: [PATCH] IB/core: Do not require CAP_NET_ADMIN for sniffing V2

Having to enable CAP_NET_ADMIN for every app that uses sniffer mode is kind
of risky. We do not want people to have the ability to mess around with the
network configuration and routing. We just want the app to direct streams and
deal with inbound data streams in various ways.

So lets drop the requirement for CAP_NET_ADMIN and keep just CAP_NET_RAW.

V1->V2
- Check for CAP_NET_ADMIN was conditional on IB_FLOW_ATTR_SNIFFER. We need
	to remove this in the correct way.
- Update description


Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>

Index: linux/drivers/infiniband/core/uverbs_cmd.c
===================================================================
--- linux.orig/drivers/infiniband/core/uverbs_cmd.c	2016-03-24 09:16:27.782778586 -0500
+++ linux/drivers/infiniband/core/uverbs_cmd.c	2016-05-13 10:49:28.953000945 -0500
@@ -3088,8 +3088,7 @@ int ib_uverbs_ex_create_flow(struct ib_u
 	if (cmd.comp_mask)
 		return -EINVAL;

-	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
-	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
+	if (!capable(CAP_NET_RAW))
 		return -EPERM;

 	if (cmd.flow_attr.flags >= IB_FLOW_ATTR_FLAGS_RESERVED)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On 05/13/2016 11:52 AM, Christoph Lameter wrote:
> Sorry slight mistake in the original patch. V2 follows
> 
> 
> 
> Subject: [PATCH] IB/core: Do not require CAP_NET_ADMIN for sniffing V2
> 
> Having to enable CAP_NET_ADMIN for every app that uses sniffer mode is kind
> of risky. We do not want people to have the ability to mess around with the
> network configuration and routing. We just want the app to direct streams and
> deal with inbound data streams in various ways.
> 
> So lets drop the requirement for CAP_NET_ADMIN and keep just CAP_NET_RAW.
> 
> V1->V2
> - Check for CAP_NET_ADMIN was conditional on IB_FLOW_ATTR_SNIFFER. We need
> 	to remove this in the correct way.
> - Update description
> 
> 
> Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
> 
> Index: linux/drivers/infiniband/core/uverbs_cmd.c
> ===================================================================
> --- linux.orig/drivers/infiniband/core/uverbs_cmd.c	2016-03-24 09:16:27.782778586 -0500
> +++ linux/drivers/infiniband/core/uverbs_cmd.c	2016-05-13 10:49:28.953000945 -0500
> @@ -3088,8 +3088,7 @@ int ib_uverbs_ex_create_flow(struct ib_u
>  	if (cmd.comp_mask)
>  		return -EINVAL;
> 
> -	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
> -	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
> +	if (!capable(CAP_NET_RAW))
>  		return -EPERM;
> 
>  	if (cmd.flow_attr.flags >= IB_FLOW_ATTR_FLAGS_RESERVED)
> 

I'm not at all convinced this is the right thing to do.  Sniffing of
packets is definitely a privileged operation.  Tcpdump needs to be run
as root to do this on regular devices.  If not CAP_NET_ADMIN, then a
root check seems appropriate.  CAP_NET_RAW does not seem sufficient for
sniffing other people's packets.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Christoph Lameter]

On Fri, 13 May 2016, Doug Ledford wrote:

> >  	if (cmd.flow_attr.flags >= IB_FLOW_ATTR_FLAGS_RESERVED)
> >
>
> I'm not at all convinced this is the right thing to do.  Sniffing of
> packets is definitely a privileged operation.  Tcpdump needs to be run
> as root to do this on regular devices.  If not CAP_NET_ADMIN, then a
> root check seems appropriate.  CAP_NET_RAW does not seem sufficient for
> sniffing other people's packets.

CAP_NET_RAW is sufficient for tcpdump and this is not more than that.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On 05/16/2016 09:53 AM, Christoph Lameter wrote:
> On Fri, 13 May 2016, Doug Ledford wrote:
> 
>>>  	if (cmd.flow_attr.flags >= IB_FLOW_ATTR_FLAGS_RESERVED)
>>>
>>
>> I'm not at all convinced this is the right thing to do.  Sniffing of
>> packets is definitely a privileged operation.  Tcpdump needs to be run
>> as root to do this on regular devices.  If not CAP_NET_ADMIN, then a
>> root check seems appropriate.  CAP_NET_RAW does not seem sufficient for
>> sniffing other people's packets.
> 
> CAP_NET_RAW is sufficient for tcpdump and this is not more than that.


That's not true.  In order to set promisc on an interface, tcpdump calls
ioctl with SIOCSIFFLAGS as the ioctl and IFF_PROMISC set in the flags,
and in net/core/dev_ioctl.c we see this check for SIOCSIFFLAGS:


               if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
                        return -EPERM;
                /* fall through */


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Christoph Lameter]

On Mon, 16 May 2016, Doug Ledford wrote:

> > CAP_NET_RAW is sufficient for tcpdump and this is not more than that.
>
>
> That's not true.  In order to set promisc on an interface, tcpdump calls
> ioctl with SIOCSIFFLAGS as the ioctl and IFF_PROMISC set in the flags,
> and in net/core/dev_ioctl.c we see this check for SIOCSIFFLAGS:

Tcpdump works fine without promiscuous mode. DONT_TRAP does not
mean that the interface is switched into promiscuous mode. It just means
that all traffic accepted by the hardware is also going to the QP
specified.

One can switch the interface into promiscuous mode in addition if one
wants that. Then you need CAP_NET_ADMIN.




--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On 05/16/2016 12:28 PM, Christoph Lameter wrote:
> On Mon, 16 May 2016, Doug Ledford wrote:
> 
>>> CAP_NET_RAW is sufficient for tcpdump and this is not more than that.
>>
>>
>> That's not true.  In order to set promisc on an interface, tcpdump calls
>> ioctl with SIOCSIFFLAGS as the ioctl and IFF_PROMISC set in the flags,
>> and in net/core/dev_ioctl.c we see this check for SIOCSIFFLAGS:
> 
> Tcpdump works fine without promiscuous mode. DONT_TRAP does not
> mean that the interface is switched into promiscuous mode. It just means
> that all traffic accepted by the hardware is also going to the QP
> specified.

You're right, I was thinking about snooping as being more than just what
it is here.

> One can switch the interface into promiscuous mode in addition if one
> wants that. Then you need CAP_NET_ADMIN.
> 
> 
> 
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On 05/16/2016 12:28 PM, Christoph Lameter wrote:
> On Mon, 16 May 2016, Doug Ledford wrote:
> 
>>> CAP_NET_RAW is sufficient for tcpdump and this is not more than that.
>>
>>
>> That's not true.  In order to set promisc on an interface, tcpdump calls
>> ioctl with SIOCSIFFLAGS as the ioctl and IFF_PROMISC set in the flags,
>> and in net/core/dev_ioctl.c we see this check for SIOCSIFFLAGS:
> 
> Tcpdump works fine without promiscuous mode. DONT_TRAP does not
> mean that the interface is switched into promiscuous mode. It just means
> that all traffic accepted by the hardware is also going to the QP
> specified.
> 
> One can switch the interface into promiscuous mode in addition if one
> wants that. Then you need CAP_NET_ADMIN.

I've applied your patch.  I completely reworded the commit message,
you'll want to check that it meets your satisfaction before I issue my
pull request.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Christoph Lameter]

On Wed, 18 May 2016, Doug Ledford wrote:

> > One can switch the interface into promiscuous mode in addition if one
> > wants that. Then you need CAP_NET_ADMIN.
>
> I've applied your patch.  I completely reworded the commit message,
> you'll want to check that it meets your satisfaction before I issue my
> pull request.

I wish I knew how to view the patch. Its not in the kernel.org git repo.
Could you post the patches you edit or provide a link to them. See how
Andrew Morton does it.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On 05/18/2016 02:12 PM, Christoph Lameter wrote:
> On Wed, 18 May 2016, Doug Ledford wrote:
> 
>>> One can switch the interface into promiscuous mode in addition if one
>>> wants that. Then you need CAP_NET_ADMIN.
>>
>> I've applied your patch.  I completely reworded the commit message,
>> you'll want to check that it meets your satisfaction before I issue my
>> pull request.
> 
> I wish I knew how to view the patch. Its not in the kernel.org git repo.
> Could you post the patches you edit or provide a link to them. See how
> Andrew Morton does it.
> 

It's in *my* kernel.org repo.  Where you trying to see it in Linus' repo?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] IB/core: Do not require CAP_NET_ADMIN for flow steering

[Christoph Lameter]

On Wed, 18 May 2016, Doug Ledford wrote:

> On 05/18/2016 02:12 PM, Christoph Lameter wrote:
> > On Wed, 18 May 2016, Doug Ledford wrote:
> >
> >>> One can switch the interface into promiscuous mode in addition if one
> >>> wants that. Then you need CAP_NET_ADMIN.
> >>
> >> I've applied your patch.  I completely reworded the commit message,
> >> you'll want to check that it meets your satisfaction before I issue my
> >> pull request.
> >
> > I wish I knew how to view the patch. Its not in the kernel.org git repo.
> > Could you post the patches you edit or provide a link to them. See how
> > Andrew Morton does it.
> >
>
> It's in *my* kernel.org repo.  Where you trying to see it in Linus' repo?

Why would I be looking at Linus'ses tree?

Ahh.. just an update issue I guess.

Changelog is ok. Just a note that there are no levels of capabilities.
CAP_NET_ADMIN covers different things from CAP_NET_RAW. It is not a higher
level.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html