netdev - Re: [PATCH net-next 5/6] netlink: add mask validation

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]

Message-ID: <20201005194759.fsszdioltmdy3u3c@lion.mk-sys.cz>
Date:   Mon, 5 Oct 2020 21:47:59 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Johannes Berg <johannes@...solutions.net>, davem@...emloft.net,
        netdev@...r.kernel.org, kernel-team@...com, jiri@...nulli.us,
        andrew@...n.ch, dsahern@...il.com, pablo@...filter.org
Subject: Re: [PATCH net-next 5/6] netlink: add mask validation

On Mon, Oct 05, 2020 at 12:34:14PM -0700, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 21:25:57 +0200 Johannes Berg wrote:
> > On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote:
> > 
> > > > > +	if (value & ~(u64)pt->mask) {
> > > > > +		NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > > > +		return -EINVAL;    
> > > > 
> > > > You had an export of the valid bits there in ethtool, using the cookie.
> > > > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > > > that seems a bit strange, but we could easily define a different attr?
> > > > 
> > > > OTOH, one can always query the policy export too (which hopefully got
> > > > wired up) so it wouldn't really matter much.  
> > > 
> > > My thinking is that there are no known uses of the cookie, it'd only
> > > have practical use to test for new flags - and we're adding first new
> > > flag in 5.10.  
> > 
> > Hm, wait, not sure I understand?
> > 
> > You _had_ this in ethtool, but you removed it now. And you're not
> > keeping it here, afaict.
> > 
> > I can't disagree on the "no known uses of the cookie" part, but it feels
> > odd to me anyway - since that is just another netlink message (*), you
> > could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the
> > data into the cookie?
> > 
> > But then are you saying the new flags are only in 5.10 so the policy
> > export will be sufficient, since that's also wired up now?
> 
> Right, I was commenting on the need to keep the cookie for backward
> compat.
> 
> My preference is to do a policy dump to check the capabilities of the
> kernel rather than shoot messages at it and then try to work backward
> based on the info returned in extack.

The reason I used the extack was that that way, the "standard" case of
kernel and ethtool in sync (or even old ethtool with new kernel) would
still use one request and only new ethtool against old kernel would end
up doing two. Also, the extra request would be relatively short and so
would be the reply to it.

On the other hand, using the policy dump would allow checking not only
support for new flags but also support for new request attributes so in
the long term, it could actually make things simpler.

Michal