linux-kernel - Re: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox register access driver

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]

Date:   Thu, 24 May 2018 17:31:12 -0700
From:   Darren Hart <dvhart@...radead.org>
To:     Vadim Pasternak <vadimp@...lanox.com>
Cc:     andy.shevchenko@...il.com, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        jiri@...nulli.us, michaelsh@...lanox.com, ivecera@...hat.com
Subject: Re: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox
 register access driver

On Mon, May 07, 2018 at 06:48:54AM +0000, Vadim Pasternak wrote:
> Introduce new Mellanox platform driver to allow access to Mellanox
> programmable device register space trough sysfs interface.
> The driver purpose is to provide sysfs interface for user space for the
> registers essential for system control and monitoring.
> The sets of registers for sysfs access are supposed to be defined per
> system type bases and include the registers related to system resets
> operation, system reset causes monitoring and some kinds of mux selection.
> 
> Signed-off-by: Vadim Pasternak <vadimp@...lanox.com>
> ---

One question on the attr init which I'm not familiar with... Andy, Greg - can
you offer your opinion below...

> +static int mlxreg_io_attr_init(struct mlxreg_io_priv_data *priv)
> +{
> +	int i;
> +
> +	priv->group.attrs = devm_kzalloc(&priv->pdev->dev,
> +					 priv->pdata->counter *
> +					 sizeof(struct attribute *),
> +					 GFP_KERNEL);
> +	if (!priv->group.attrs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->pdata->counter; i++) {
> +		priv->mlxreg_io_attr[i] =
> +				&priv->mlxreg_io_dev_attr[i].dev_attr.attr;
> +
> +		/* Set attribute name as a label. */
> +		priv->mlxreg_io_attr[i]->name =
> +				devm_kasprintf(&priv->pdev->dev, GFP_KERNEL,
> +					       priv->pdata->data[i].label);
> +
> +		if (!priv->mlxreg_io_attr[i]->name) {
> +			dev_err(&priv->pdev->dev, "Memory allocation failed for sysfs attribute %d.\n",
> +				i + 1);
> +			return -ENOMEM;
> +		}
> +
> +		priv->mlxreg_io_dev_attr[i].dev_attr.attr.mode =
> +						priv->pdata->data[i].mode;
> +		switch (priv->pdata->data[i].mode) {

This seemed a bit odd to me. Do we need to do this conditional assignment within
the kernel, or can these just be assigned, and the mode will guard against the
user being able to call store on a read only attr?

> +		case 0200:
> +			priv->mlxreg_io_dev_attr[i].dev_attr.store =
> +							mlxreg_io_attr_store;
> +			break;
> +
> +		case 0444:
> +			priv->mlxreg_io_dev_attr[i].dev_attr.show =
> +							mlxreg_io_attr_show;
> +			break;
> +
> +		case 0644:
> +			priv->mlxreg_io_dev_attr[i].dev_attr.show =
> +							mlxreg_io_attr_show;
> +			priv->mlxreg_io_dev_attr[i].dev_attr.store =
> +							mlxreg_io_attr_store;
> +			break;

If this is necessary, we can simplify this by checking for the read mask and the
write mask and setting each once - rather than duplicating this for r, w, and
rw. As it is a 0400 would not assign the show function, even though it is
readable by somebody.

-- 
Darren Hart
VMware Open Source Technology Center