netdev - Re: [PATCH] Code changes to handle dev private ioctl

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: <1243002079.3192.11.camel@achroite>
Date:	Fri, 22 May 2009 15:21:19 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Sarveshwar Bandi <sarveshwarb@...verengines.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] Code changes to handle dev private ioctl

On Fri, 2009-05-22 at 19:03 +0530, Sarveshwar Bandi wrote:
[...]
> diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> index ae2f6b5..8329e07 100644
> --- a/drivers/net/benet/be_main.c
> +++ b/drivers/net/benet/be_main.c
> @@ -562,6 +562,53 @@ static void be_vlan_rem_vid(struct net_d
>  	be_vid_config(netdev);
>  }
>  
> +static int be_do_ioctl(struct net_device *netdev,
> +			struct ifreq *ifr, int cmd)
> +{
> +	struct be_adapter *adapter = netdev_priv(netdev);
> +	struct be_cmd_req_hdr req;
> +	struct be_cmd_resp_hdr *resp;
> +	void *data = ifr->ifr_data;
> +	void *va;
> +	dma_addr_t dma;
> +	u32 req_size, resp_size;
> +	int status;
> +
> +	switch (cmd) {
> +	case SIOCDEVPRIVATE:

This probably needs a capability check, depending on what it's actually
doing.  Most configuration ioctl implementation use:

		if (!capable(CAP_NET_ADMIN))
			return -EPERM;

> +		if (copy_from_user(&req, (struct be_cmd_req_hdr *)data,
> +			sizeof(struct be_cmd_req_hdr)))
> +			return -EFAULT;
> +
> +		req_size = req.request_length + sizeof(struct be_cmd_req_hdr);
> +
> +		va = pci_alloc_consistent(adapter->pdev, req_size, &dma);

Needs an error check.

> +		if (copy_from_user(va, (void *)data, req_size)) {
> +			pci_free_consistent(adapter->pdev, req_size, va, dma);
> +			return -EFAULT;
> +		}
> +
> +		status = be_cmd_pass_ext_ioctl(&adapter->ctrl, dma, req_size);
> +		if (!status) {
> +			resp = (struct be_cmd_resp_hdr *) va;
> +			resp_size = resp->response_length + sizeof(*resp);
> +			if (copy_to_user((void *)data, va, resp_size)) {
> +				pci_free_consistent(adapter->pdev,
> +							req_size, va, dma);
> +				return -EFAULT;
> +			}
> +		} else {
> +			pci_free_consistent(adapter->pdev, req_size, va, dma);
> +			return -EFAULT;

Why repeat this cleanup and return so many times?  Kernel style is to
put cleanup code at the end of the function and use "goto" to get there
in case of error.

> +		}
> +		pci_free_consistent(adapter->pdev, req_size, va, dma);
> +		break;
> +	default:
> +		return -EFAULT;
[...]

Should be -EOPNOTSUPP.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html