linux-kernel - Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component

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: <87y4aq2bjj.fsf@ashishki-desk.ger.corp.intel.com>
Date:	Fri, 12 Feb 2016 17:28:16 +0200
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	Chunyan Zhang <zhang.chunyan@...aro.org>,
	mathieu.poirier@...aro.org
Cc:	mike.leach@....com, Michael.Williams@....com, al.grant@....com,
	tor@...com, nicolas.guion@...com, broonie@...nel.org,
	robh@...nel.org, pratikp@...eaurora.org, corbet@....net,
	mark.rutland@....com, zhang.lyra@...il.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-api@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component

Chunyan Zhang <zhang.chunyan@...aro.org> writes:

> +static long stm_generic_set_options(struct stm_data *stm_data,
> +				    unsigned int master,
> +				    unsigned int channel,
> +				    unsigned int nr_chans,
> +				    unsigned long options)
> +{
> +	struct stm_drvdata *drvdata = container_of(stm_data,
> +						   struct stm_drvdata, stm);
> +	if (!(drvdata && drvdata->enable))
> +		return -EINVAL;
> +
> +	if (channel >= drvdata->numsp)
> +		return -EINVAL;
> +
> +	switch (options) {
> +	case STM_OPTION_GUARANTEED:
> +		set_bit(channel, drvdata->chs.guaranteed);
> +		break;
> +
> +	case STM_OPTION_INVARIANT:
> +		clear_bit(channel, drvdata->chs.guaranteed);
> +		break;

This is a bad interface. Firstly, neither option is described
anywhere. Secondly, I'm pretty sure "invariant" does not mean "not
guaranteed" in english, although this function seems to imply this.

> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static long stm_generic_get_options(struct stm_data *stm_data,
> +				    unsigned int master,
> +				    unsigned int channel,
> +				    unsigned int nr_chans,
> +				    u64 *options)
> +{
> +	struct stm_drvdata *drvdata = container_of(stm_data,
> +						   struct stm_drvdata, stm);
> +	if (!(drvdata && drvdata->enable))
> +		return -EINVAL;
> +
> +	if (channel >= drvdata->numsp)
> +		return -EINVAL;
> +
> +	switch (*options) {
> +	case STM_OPTION_GUARANTEED:
> +		*options = test_bit(channel, drvdata->chs.guaranteed);
> +		break;

This just doesn't work. @options here is an on-stack variable in
stm_char_ioctl(), hitherto uninitialized. The get_options ioctl command
as you implemented it doesn't fetch @options from userspace either, it
just passes a pointer to it to this callback, expecting that the
callback will set it so that it can be copy_to_user()ed back to the
user.

Then, when we figure this out, there is again the question of what
should one make of STM_OPTION_{GUARANTEED,INVARIANT} and how do they fit
into *options.

The idea behind set_options ioctl is that the user specifies a bit mask
of options that he/she wants to set.

[snip]

> +#ifndef __UAPI_CORESIGHT_STM_H_
> +#define __UAPI_CORESIGHT_STM_H_
> +
> +#define STM_FLAG_TIMESTAMPED   BIT(3)
> +#define STM_FLAG_GUARANTEED    BIT(7)
> +
> +enum {
> +	STM_OPTION_GUARANTEED = 0,
> +	STM_OPTION_INVARIANT,
> +};

Each of these guys could also use an explanation.

Regards,
--
Alex