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:   Sat, 26 May 2018 11:15:35 +0000
From:   Vadim Pasternak <vadimp@...lanox.com>
To:     Darren Hart <dvhart@...radead.org>
CC:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        Michael Shych <michaelsh@...lanox.com>,
        "ivecera@...hat.com" <ivecera@...hat.com>
Subject: RE: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox
 register access driver



> -----Original Message-----
> From: Darren Hart [mailto:dvhart@...radead.org]
> Sent: Friday, May 25, 2018 3:31 AM
> 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;
> Michael Shych <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.

Maybe I really can add something like
static struct device_attribute mlxreg_io_devattr_rw = {
	.show	= mlxreg_io_attr_show,
	.store	= mlxreg_io_attr_store,
};

And replace this whole switch statement just with:
		memcpy(&priv->mlxreg_io_dev_attr[i].dev_attr,
		       &mlxreg_io_devattr_rw, sizeof(struct device_attribute));
> 
> --
> Darren Hart
> VMware Open Source Technology Center