Re: [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Jan 23, 2021 at 9:29 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
>
> This RFC is a change in approach from my previous RFC patch [1]. It adds
> "pinnux-select" to debugfs. Function and group on the pin control device
> will be activated when 2 integers "<function-selector> <group-selector>"
> are written to the file. The debugfs write operation pinmux_select()
> handles this by calling ops->set_mux() with fsel and gsel.

...

> RFC notes:

Please, move below to reST formatted document.

...

> +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> +                                  size_t cnt, loff_t *ppos)
> +{
> +       struct seq_file *sfile = file->private_data;
> +       struct pinctrl_dev *pctldev = sfile->private;
> +       const struct pinmux_ops *ops = pctldev->desc->pmxops;
> +       int fsel, gsel, ret;
> +       // RFC note: two integers separated by a space should never exceed 16
> +       char buf[16];

> +       if (*ppos != 0)
> +               return -EINVAL;

But why? Do we really care about it? Moreover, you have no_llseek() below.

> +       ret = strncpy_from_user(buf, user_buf, cnt);

Potential buffer overflow.

  cnt -> sizeof(buf)

> +       if (ret < 0)
> +               return ret;

> +       buf[cnt] = '\0';

Not sure, shouldn't be

  buf[sizeof(buf) - 1] = '\0';

?

> +       if (buf[cnt - 1] == '\n')
> +               buf[cnt - 1] = '\0';

strstrip() ?

> +       ret = sscanf(buf, "%d %d", &fsel, &gsel);
> +       if (ret != 2) {

> +               dev_err(pctldev->dev, "%s: sscanf() expects '<fsel> <gsel>'", __func__);

__func__ is useless, please drop it. And below as well.

> +               return -EINVAL;
> +       }

> +       ret = ops->set_mux(pctldev, fsel, gsel);
> +       if (ret != 0) {

I thought I gave you a comment on this...

if (ret)

> +               dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret);
> +               return -EINVAL;
> +       }
> +
> +       return cnt;
> +}

...

>         debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO,
>                             devroot, pctldev, &pinmux_pins_fops);
> +       debugfs_create_file("pinmux-select", 0200,
> +                           devroot, pctldev, &pinmux_set_ops);

Consider to add another (prerequisite) patch to get rid of symbolic permissions.

-- 
With Best Regards,
Andy Shevchenko