[PATCH] cxgb4: use kstrtou16_from_user() in tp_la_write()

[PATCH] cxgb4: use kstrtou16_from_user() in tp_la_write()

[Dmitry Antipov]

Since actual value used for 'la_mask' can't exceed 0xffff, it
doesn't make sense to parse larger values as 'unsigned long'.
So switch to 'u16' to store an expected value and use the
convenient 'kstrtou16_from_user()' to simplify 'tp_la_write()'

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index f521737d1275..5c6c27418891 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -664,20 +664,13 @@ static ssize_t tp_la_write(struct file *file, const char __user *buf,
 			   size_t count, loff_t *pos)
 {
 	int err;
-	char s[32];
-	unsigned long val;
-	size_t size = min(sizeof(s) - 1, count);
+	u16 val;
 	struct adapter *adap = file_inode(file)->i_private;
 
-	if (copy_from_user(s, buf, size))
-		return -EFAULT;
-	s[size] = '\0';
-	err = kstrtoul(s, 0, &val);
+	err = kstrtou16_from_user(buf, count, 0, &val);
 	if (err)
 		return err;
-	if (val > 0xffff)
-		return -EINVAL;
-	adap->params.tp.la_mask = val << 16;
+	adap->params.tp.la_mask = (unsigned int)val << 16;
 	t4_set_reg_field(adap, TP_DBG_LA_CONFIG_A, 0xffff0000U,
 			 adap->params.tp.la_mask);
 	return count;
-- 
2.55.0

Re: [PATCH] cxgb4: use kstrtou16_from_user() in tp_la_write()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
cxgb4: use kstrtou16_from_user() in tp_la_write()

This replaces the open-coded copy_from_user() plus kstrtoul() parsing in
tp_la_write() with kstrtou16_from_user(), storing the parsed value in a u16
and dropping the explicit val > 0xffff check.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> index f521737d12756..5c6c274188913 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> @@ -664,20 +664,13 @@ static ssize_t tp_la_write(struct file *file, const char __user *buf,
>  			   size_t count, loff_t *pos)
>  {
>  	int err;
> -	char s[32];
> -	unsigned long val;
> -	size_t size = min(sizeof(s) - 1, count);
> +	u16 val;
>  	struct adapter *adap = file_inode(file)->i_private;
>  
> -	if (copy_from_user(s, buf, size))
> -		return -EFAULT;
> -	s[size] = '\0';
> -	err = kstrtoul(s, 0, &val);
> +	err = kstrtou16_from_user(buf, count, 0, &val);
>  	if (err)
>  		return err;
> -	if (val > 0xffff)
> -		return -EINVAL;
> -	adap->params.tp.la_mask = val << 16;
> +	adap->params.tp.la_mask = (unsigned int)val << 16;

[Severity: Low]
Does this change the accepted input length and the returned errno for
tp_la_write()?

The helper instantiated for u16 in lib/kstrtox.c sizes its buffer from the
target type:

#define kstrto_from_user(f, g, type)					\
int f(const char __user *s, size_t count, unsigned int base, type *res)	\
{									\
	/* sign, base 2 representation, newline, terminator */		\
	char buf[1 + sizeof(type) * 8 + 1 + 1];				\
									\
	count = min(count, sizeof(buf) - 1);				\
	if (copy_from_user(buf, s, count))				\
		return -EFAULT;						\
	buf[count] = '\0';						\
	return g(buf, base, res);					\
}									\
...
kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);

so buf is 19 bytes and the input is clamped to 18 bytes, where the previous
code used char s[32] and accepted up to 31 characters.

For a write longer than 18 bytes, such as a zero-padded
"0x0000000000000000ffff", the clamp keeps only the 18-byte prefix, a
different value is programmed into TP_DBG_LA_CONFIG_A by
t4_set_reg_field(), and tp_la_write() still returns the full count, so the
write looks successful.  Any invalid characters past byte 18 are also never
seen by the parser.  Should the handler reject count larger than what the
helper can consume instead?

Separately, values above 0xffff now come back as -ERANGE from kstrtou16()
rather than the -EINVAL that the removed check produced:

-	if (val > 0xffff)
-		return -EINVAL;

Could the commit message mention the new 18-byte input limit and the errno
change, since it currently describes the change only as a way to
"simplify 'tp_la_write()'"?
-- 
pw-bot: cr