linux-kernel - Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command

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: <f54fcb1e9f90b529826d8d6abb2ae99b15108d16.camel@perches.com>
Date:   Wed, 09 Dec 2020 00:30:46 -0800
From:   Joe Perches <joe@...ches.com>
To:     mgross@...ux.intel.com, markgross@...nel.org, arnd@...db.de,
        bp@...e.de, damien.lemoal@....com, dragan.cvetic@...inx.com,
        gregkh@...uxfoundation.org, corbet@....net,
        leonard.crestez@....com, palmerdabbelt@...gle.com,
        paul.walmsley@...ive.com, peng.fan@....com, robh+dt@...nel.org,
        shawnguo@...nel.org
Cc:     linux-kernel@...r.kernel.org, Seamus Kelly <seamus.kelly@...el.com>
Subject: Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by
 creating sub-functions for each ioctl command

On Tue, 2020-12-01 at 14:35 -0800, mgross@...ux.intel.com wrote:
> Refactor the too large IOCTL function to call helper functions.
[]
> diff --git a/drivers/misc/xlink-core/xlink-ioctl.c b/drivers/misc/xlink-core/xlink-ioctl.c
[]
> +int ioctl_write_data(unsigned long arg)
> +{
> +	struct xlink_handle	devh	= {0};
> +	struct xlinkwritedata		wr	= {0};
> +	int rc = 0;
> +
> +	if (copy_from_user(&wr, (void __user *)arg,
> +			   sizeof(struct xlinkwritedata)))
> +		return -EFAULT;
> +	if (copy_from_user(&devh, (void __user *)wr.handle,
> +			   sizeof(struct xlink_handle)))
> +		return -EFAULT;
> +	if (wr.size <= XLINK_MAX_DATA_SIZE) {
> +		rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage,
> +					   wr.size);
> +		if (copy_to_user((void __user *)wr.return_code, (void *)&rc,
> +				 sizeof(rc)))
> +			return -EFAULT;
> +	} else {
> +		return -EFAULT;
> +	}

Please reverse the test to reduce indentation

	if (wr.size > XLINK_MAX_DATA_SIZE)
		return -EFAULT;
	rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);
	if (copy_to_user((void __user *)wr.return_code, (void *)&rc, sizeof(rc)))
		return -EFAULT;
	return rc;

The last 3 lines here are repeated multiple times in many functions.
It might be sensible to add something like:

int copy_result_to_user(u32 *where, int rc)
{
	if (copy_to_user((void __user *)where, &rc, sizeof(rc)))
		return -EFAULT;
	return rc;
}

so this could be written

	rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);

	return copy_result_to_user(wr.return_code, rc);

IMO: return_code isn't a great name for a pointer as it rather
indicates a value not an address and there's an awful lot of
casting to __user in all this code that perhaps should be marked
in the struct definitions rather than inside the function uses.

> +}
> +
> +int ioctl_write_control_data(unsigned long arg)
> +{
> +	struct xlink_handle	devh	= {0};

All of these initializations with {0} should use {} instead as
the first element of whatever struct is not guaranteed to be
assignable as an int and gcc/clang guarantee 0 initialization

> +	struct xlinkwritedata		wr	= {0};
> +	u8 volbuf[XLINK_MAX_BUF_SIZE];
> +	int rc = 0;
> +
> +	if (copy_from_user(&wr, (void __user *)arg,
> +			   sizeof(struct xlinkwritedata)))
> +		return -EFAULT;
> +	if (copy_from_user(&devh, (void __user *)wr.handle,
> +			   sizeof(struct xlink_handle)))
> +		return -EFAULT;
> +	if (wr.size <= XLINK_MAX_CONTROL_DATA_SIZE) {
> +		if (copy_from_user(volbuf, (void __user *)wr.pmessage,
> +				   wr.size))
> +			return -EFAULT;
> +		rc = xlink_write_control_data(&devh, wr.chan, volbuf,
> +					      wr.size);
> +		if (copy_to_user((void __user *)wr.return_code,
> +				 (void *)&rc, sizeof(rc)))
> +			return -EFAULT;
> +	} else {
> +		return -EFAULT;

Same test reversal and deindentation please.

> +	}
> +	return rc;
> +}
> +