[PATCH v1 1/2] pinctrl: Duplicate user memory in one go in pinmux_select()

[PATCH v1 1/2] pinctrl: Duplicate user memory in one go in pinmux_select()

[Andy Shevchenko]

Current code is suboptimal in three ways:
1) it explicitly terminates the string which is not needed;
2) it might provoke additional faults, because asked lenght might be
   bigger than the real one;
3) it consumes more than needed lines in the source.

Instead of using kmalloc() + strncpy_from_user() + terminating, just
utilize memdup_user_nul().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinmux.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 021382632608..2d2f3bd164d5 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -692,14 +692,9 @@ static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
 	if (len > PINMUX_SELECT_MAX)
 		return -ENOMEM;
 
-	buf = kzalloc(PINMUX_SELECT_MAX, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = strncpy_from_user(buf, user_buf, PINMUX_SELECT_MAX);
-	if (ret < 0)
-		goto exit_free_buf;
-	buf[len-1] = '\0';
+	buf = memdup_user_nul(user_buf, len);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
 
 	/* remove leading and trailing spaces of input buffer */
 	gname = strstrip(buf);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/2] pinctrl: Relax user input size in pinmux_select()

[Andy Shevchenko]

This is debugfs and there is no much sense to strict the user from
sending as much data as they can. The memdup_user_nul() will anyway
fail if there is not enough memory.

Relax the user input size by removing an artificial limitaion.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinmux.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 2d2f3bd164d5..82c750a31952 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -677,7 +677,6 @@ void pinmux_show_setting(struct seq_file *s,
 DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
 DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
 
-#define PINMUX_SELECT_MAX 128
 static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
 				   size_t len, loff_t *ppos)
 {
@@ -689,9 +688,6 @@ static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
 	unsigned int num_groups;
 	int fsel, gsel, ret;
 
-	if (len > PINMUX_SELECT_MAX)
-		return -ENOMEM;
-
 	buf = memdup_user_nul(user_buf, len);
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/2] pinctrl: Duplicate user memory in one go in pinmux_select()

[Linus Walleij]

On Sun, Jun 4, 2023 at 3:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Current code is suboptimal in three ways:
> 1) it explicitly terminates the string which is not needed;
> 2) it might provoke additional faults, because asked lenght might be
>    bigger than the real one;
> 3) it consumes more than needed lines in the source.
>
> Instead of using kmalloc() + strncpy_from_user() + terminating, just
> utilize memdup_user_nul().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Excellent patch, applied!

Yours,
Linus Walleij

Re: [PATCH v1 2/2] pinctrl: Relax user input size in pinmux_select()

[Linus Walleij]

On Sun, Jun 4, 2023 at 3:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> This is debugfs and there is no much sense to strict the user from
> sending as much data as they can. The memdup_user_nul() will anyway
> fail if there is not enough memory.
>
> Relax the user input size by removing an artificial limitaion.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Fair enough, patch applied!

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: Relax user input size in pinmux_select()

[Andy Shevchenko]

On Tue, Jun 13, 2023 at 04:38:17PM +0200, Markus Elfring wrote:
> > This is debugfs and there is no much sense to strict the user from
> > sending as much data as they can. The memdup_user_nul() will anyway
> > fail if there is not enough memory.
> >
> > Relax the user input size by removing an artificial limitaion.
> 
> How are the chances to avoid typos also in such a change description?

Almost 0. This is now part of the non-rebased branch. It can be fixed
if and only if
- something really wrong happened before this patch in the tree and has
  to be fixed, and
- maintainer will remember to fix the typo.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: Duplicate user memory in one go in pinmux_select()

[Andy Shevchenko]

On Tue, Jun 13, 2023 at 04:30:44PM +0200, Markus Elfring wrote:
> > …, because asked lenght …
> 
> * Are there any chances to avoid a typo in such a change description?

Not anymore as explained in the other email, but thanks for pointing this out.

> * Was a cover letter accidentally omitted?

What do you want to see in such cover letter? How can it be helpful?
(Note that these are rhetorical towards this change, as it's already
 in the non-rebased branch)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pinctrl: Relax user input size in pinmux_select()

[Linus Walleij]

On Tue, Jun 13, 2023 at 4:38 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> > This is debugfs and there is no much sense to strict the user from
> > sending as much data as they can. The memdup_user_nul() will anyway
> > fail if there is not enough memory.
> >
> > Relax the user input size by removing an artificial limitaion.
>
> How are the chances to avoid typos also in such a change description?

I don't care much about typos, and when a patch is coming from Andy
it's certainly not a blocker for anything since his patches are
of high technical value.

Yours,
Linus Walleij