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: <20201005124029.5ebe684d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 5 Oct 2020 12:40:29 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Michal Kubecek <mkubecek@...e.cz>, 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, 05 Oct 2020 21:31:13 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 21:28 +0200, Michal Kubecek 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?  
> > 
> > The idea behind the cookie was that if new userspace sends a request
> > with multiple flags which may not be supported by an old kernel, getting
> > only -EOPNOTSUPP (and badattr pointing to the flags) would not be very
> > helpful as multiple iteration would be necessary to find out which flags
> > are supported and which not.  
> 
> Message crossing, I guess.
> 
> I completely agree. I just don't like using the (somewhat vague)
> _cookie_ for that vs. adding a new explicit NLMSGERR_ATTR_SOMETHING :)
> 
> I would totally support doing that here in the general validation code,
> but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate
> attribute for it.

Hm. Perhaps we can do a partial policy dump into the extack?

NL_POLICY_TYPE_ATTR_TYPE etc.?

Either way, I don't feel like this series needs it.

> > I'm not exactly happy with the prospect of having to do a full policy
> > dump before each such request, thought.

I tried to code it up and it gets rather ugly.

One has to selectively suppress error messages for stuff that's known
to be optional, and keep retrying.

The policy dump is much, much cleaner (and the policy for an op is
rather tiny - one nested policy of the header with 3? attrs in it).