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: <18fc3d1719ce09d5aa145a164bf407fe7a7bbb81.camel@sipsolutions.net>
Date:   Mon, 05 Oct 2020 21:25:57 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, kernel-team@...com,
        jiri@...nulli.us, andrew@...n.ch, mkubecek@...e.cz,
        dsahern@...il.com, pablo@...filter.org
Subject: Re: [PATCH net-next 5/6] netlink: add mask validation

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?

johannes

(*) in a way - the ack message has the "legacy" fixed part before the
attrs, of course