netdev - Re: [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled

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]

Message-ID: <20190104112532.GA3471@splinter>
Date:   Fri, 4 Jan 2019 11:25:34 +0000
From:   Ido Schimmel <idosch@...lanox.com>
To:     David Ahern <dsahern@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>
Subject: Re: [PATCH iproute2-next] bridge: fdb: Fix filtering with strict
 checking enabled / disabled

On Wed, Jan 02, 2019 at 09:34:28PM -0700, David Ahern wrote:
> On 1/1/19 12:18 AM, Ido Schimmel wrote:
> > When strict checking is enabled the kernel expects to receive the
> > ifindex of the bridge device using 'NDA_MASTER', but iproute2 currently
> > uses 'IFLA_MASTER' which the kernel expects when strict checking is
> > disabled. Therefore, using iproute2 on current kernels while filtering
> > on bridge results in the following error:
> > 
> > # bridge fdb show br br0
> > Error: Unsupported attribute in fdb dump request.
> > Dump terminated
> > 
> > Additionally, when strict checking is disabled and the bridge is
> > specified via 'IFLA_MASTER', we need to make sure that the message
> > length actually corresponds to 'struct ifinfomsg' and a potential
> > attribute.
> > 
> > Fix this by adding a new flag to the RTNL handle which indicates whether
> > strict checking is enabled on the socket or not. If it is enabled,
> > specify 'NDA_MASTER'. Otherwise, specify 'IFLA_MASTER' and set the
> > message length accordingly.
> > 
> > Tested with and without strict checking on net-next and on older kernels
> > (v4.18, v4.17, v4.9).
> > 
> > Fixes: 66e8e73edc65 ("bridge: fdb: Use 'struct ndmsg' for FDB dumping")
> > Fixes: aea41afcfd6d ("ip bridge: Set NETLINK_GET_STRICT_CHK on socket")
> > Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> > ---
> >  bridge/fdb.c         | 11 ++++++++++-
> >  include/libnetlink.h |  1 +
> >  lib/libnetlink.c     |  6 ++++--
> >  3 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/bridge/fdb.c b/bridge/fdb.c
> > index a7a0d8052307..f898b20918fb 100644 I
> > --- a/bridge/fdb.c
> > +++ b/bridge/fdb.c
> > @@ -271,6 +271,11 @@ static int fdb_show(int argc, char **argv)
> >  	char *br = NULL;
> >  	int msg_size = sizeof(struct ndmsg);
> >  
> > +	if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) {
> > +		req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > +		msg_size = sizeof(struct ifinfomsg);
> > +	}
> > +
> >  	while (argc > 0) {
> >  		if ((strcmp(*argv, "brport") == 0) || strcmp(*argv, "dev") == 0) {
> >  			NEXT_ARG();
> > @@ -304,7 +309,11 @@ static int fdb_show(int argc, char **argv)
> >  			fprintf(stderr, "Cannot find bridge device \"%s\"\n", br);
> >  			return -1;
> >  		}
> > -		addattr32(&req.n, sizeof(req), IFLA_MASTER, br_ifindex);
> > +
> > +		if (rth.flags & RTNL_HANDLE_F_STRICT_CHK)
> > +			addattr32(&req.n, sizeof(req), NDA_MASTER, br_ifindex);
> > +		else
> > +			addattr32(&req.n, sizeof(req), IFLA_MASTER, br_ifindex);
> >  		msg_size += RTA_LENGTH(4);
> >  	}
> >  
> 
> I like the addition of the flag to rth as a way for commands to know if
> the kernel supports strict checking.
> 
> Couple of things. first, the patch (at least when I saved it to file)
> has html codes in it. New for a patch from you so not sure what
> happened. An example:

No clue. I didn't do anything differently.

> 
> diff --git a/bridge/fdb.c b/bridge/fdb.c
> index a7a0d8052307..f898b20918fb 100644
> --- a/bridge/fdb.c
> +++ b/bridge/fdb.c
> @@ -271,6 +271,11 @@ static int fdb_show(int argc, char **argv)
>         char *br =3D NULL;
>         int msg_size =3D sizeof(struct ndmsg);
> =20
> +       if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) {
> +               req.n.nlmsg_len =3D NLMSG_LENGTH(sizeof(struct ifinfomsg));
> +               msg_size =3D sizeof(struct ifinfomsg);
> +       }
> +
>         while (argc > 0) {
>                 if ((strcmp(*argv, "brport") =3D=3D 0) || strcmp(*argv,
> "dev") =3D=3D 0)=
>  {
>                         NEXT_ARG();
> 
> 
> Second, the current code after your last patch sets the ifindex in the
> ancillary header based on ndmsg versus ifinfomsg. While the offset is
> the same, it seems odd and a challenge for future readers that the
> master attribute toggles between IFLA and NDA but the struct entry does
> not matter.

Yes, I agree it's not pretty.

> I think long term the code should be consistent with other dump
> commands. I missed neigh and fdb dumps in my first round. Specifically,
> removing the req from the list functions and instead using the type
> specific dump functions with a filter function that can append to the
> request when it makes sense. I have that coded and seems to work fine on
> latest kernel and older (4.1). If you have some time another
> verification would be great. Thanks,

I'll take a look.