netdev - Re: [PATCH wpan-next 1/6] ieee802154: Add support for user scanning requests

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: <20230210111843.0817d0d3@xps-13>
Date:   Fri, 10 Feb 2023 11:18:43 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Alexander Aring <alex.aring@...il.com>,
        Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Guilhem Imberton <guilhem.imberton@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next 1/6] ieee802154: Add support for user scanning
 requests

Hi Stefan, Jakub,

kuba@...nel.org wrote on Fri, 3 Feb 2023 20:19:23 -0800:

> On Tue, 29 Nov 2022 17:00:41 +0100 Miquel Raynal wrote:
> > +static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> > +	struct net_device *dev = info->user_ptr[1];
> > +	struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
> > +	struct wpan_phy *wpan_phy = &rdev->wpan_phy;
> > +	struct cfg802154_scan_request *request;
> > +	u8 type;
> > +	int err;
> > +
> > +	/* Monitors are not allowed to perform scans */
> > +	if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)  
> 
> extack ?

Thanks for pointing at it, I just did know about it. I did convert
most of the printk's into extack strings. Shall I keep both or is fine
to just keep the extack thing?

For now I've dropped the printk's, please tell me if this is wrong.

> 
> > +		return -EPERM;

Stefan, do you prefer a series of patches applying on top of your
current next or should I re-roll the entire series (scan + beacons)?

I am preparing a series applying on top of the current list of applied
patches. This means next PR to net maintainers will include this patch
as it is today + fixes on top. If this is fine for both parties, I will
send these (including the other changes discussed with Alexander). Just
let me know.

Sorry btw for the delay, I really had to finish other activities before
switching back.

> > +
> > +	request = kzalloc(sizeof(*request), GFP_KERNEL);
> > +	if (!request)
> > +		return -ENOMEM;
> > +
> > +	request->wpan_dev = wpan_dev;
> > +	request->wpan_phy = wpan_phy;
> > +
> > +	type = nla_get_u8(info->attrs[NL802154_ATTR_SCAN_TYPE]);  
> 
> what checks info->attrs[NL802154_ATTR_SCAN_TYPE] is not NULL?
> 
> > +	switch (type) {
> > +	case NL802154_SCAN_PASSIVE:
> > +		request->type = type;
> > +		break;
> > +	default:
> > +		pr_err("Unsupported scan type: %d\n", type);
> > +		err = -EINVAL;  
> 
> extack (printfs are now supported)
> 
> > +		goto free_request;
> > +	}
> > +
> > +	if (info->attrs[NL802154_ATTR_PAGE]) {
> > +		request->page = nla_get_u8(info->attrs[NL802154_ATTR_PAGE]);
> > +		if (request->page > IEEE802154_MAX_PAGE) {  
> 
> bound check should be part of the policy NLA_POLICY_MAX()

I just improved the policies to make these checks useless and simplify a
lot the code there, thanks as well for pointing at it.

> > +			pr_err("Invalid page %d > %d\n",
> > +			       request->page, IEEE802154_MAX_PAGE);
> > +			err = -EINVAL;  
> 
> extack
> 
> > +			goto free_request;
> > +		}
> > +	} else {
> > +		/* Use current page by default */
> > +		request->page = wpan_phy->current_page;
> > +	}
> > +
> > +	if (info->attrs[NL802154_ATTR_SCAN_CHANNELS]) {
> > +		request->channels = nla_get_u32(info->attrs[NL802154_ATTR_SCAN_CHANNELS]);
> > +		if (request->channels >= BIT(IEEE802154_MAX_CHANNEL + 1)) {  
> 
> policy as well
> 
> > +			pr_err("Invalid channels bitfield %x ≥ %lx\n",
> > +			       request->channels,
> > +			       BIT(IEEE802154_MAX_CHANNEL + 1));
> > +			err = -EINVAL;
> > +			goto free_request;
> > +		}
> > +	} else {
> > +		/* Scan all supported channels by default */
> > +		request->channels = wpan_phy->supported.channels[request->page];
> > +	}
> > +
> > +	if (info->attrs[NL802154_ATTR_SCAN_PREAMBLE_CODES] ||
> > +	    info->attrs[NL802154_ATTR_SCAN_MEAN_PRF]) {
> > +		pr_err("Preamble codes and mean PRF not supported yet\n");  
> 
> NLA_REJECT also in policy
> 
> > +		err = -EINVAL;
> > +		goto free_request;
> > +	}
> > +
> > +	if (info->attrs[NL802154_ATTR_SCAN_DURATION]) {
> > +		request->duration = nla_get_u8(info->attrs[NL802154_ATTR_SCAN_DURATION]);
> > +		if (request->duration > IEEE802154_MAX_SCAN_DURATION) {
> > +			pr_err("Duration is out of range\n");
> > +			err = -EINVAL;
> > +			goto free_request;
> > +		}
> > +	} else {
> > +		/* Use maximum duration order by default */
> > +		request->duration = IEEE802154_MAX_SCAN_DURATION;
> > +	}
> > +
> > +	if (wpan_dev->netdev)
> > +		dev_hold(wpan_dev->netdev);  
> 
> Can we put a tracker in the request and use netdev_hold() ?

I'll look into it.

Thanks,
Miquèl