[PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy

[PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy

[Romain Perier]

strlcpy() copy a C-String into a sized buffer, the result is always a
valid NULL-terminated that fits in the buffer, howerver it has severals
issues. It reads the source buffer first, which is dangerous if it is non
NULL-terminated or if the corresponding buffer is unbounded. Its safe
replacement is strscpy(), as suggested in the deprecated interface [1].

We plan to make this contribution in two steps:
- Firsly all cases of strlcpy's return value are manually replaced by the
  corresponding calls of strscpy() with the new handling of the return
  value (as the return code is different in case of error).
- Then all other cases are automatically replaced by using coccinelle.

This series covers manual replacements.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Romain Perier (20):
  cgroup: Manual replacement of the deprecated strlcpy() with return
    values
  crypto: Manual replacement of the deprecated strlcpy() with return
    values
  devlink: Manual replacement of the deprecated strlcpy() with return
    values
  dma-buf: Manual replacement of the deprecated strlcpy() with return
    values
  kobject: Manual replacement of the deprecated strlcpy() with return
    values
  ima: Manual replacement of the deprecated strlcpy() with return values
  SUNRPC: Manual replacement of the deprecated strlcpy() with return
    values
  kernfs: Manual replacement of the deprecated strlcpy() with return
    values
  m68k/atari: Manual replacement of the deprecated strlcpy() with return
    values
  module: Manual replacement of the deprecated strlcpy() with return
    values
  hwmon: Manual replacement of the deprecated strlcpy() with return
    values
  s390/hmcdrv: Manual replacement of the deprecated strlcpy() with
    return values
  scsi: zfcp: Manual replacement of the deprecated strlcpy() with return
    values
  target: Manual replacement of the deprecated strlcpy() with return
    values
  ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with
    return values
  tracing/probe: Manual replacement of the deprecated strlcpy() with
    return values
  vt: Manual replacement of the deprecated strlcpy() with return values
  usb: gadget: f_midi: Manual replacement of the deprecated strlcpy()
    with return values
  usbip: usbip_host: Manual replacement of the deprecated strlcpy() with
    return values
  s390/watchdog: Manual replacement of the deprecated strlcpy() with
    return values

 arch/m68k/emu/natfeat.c                 |  6 +--
 crypto/lrw.c                            |  6 +--
 crypto/xts.c                            |  6 +--
 drivers/dma-buf/dma-buf.c               |  4 +-
 drivers/hwmon/pmbus/max20730.c          | 66 +++++++++++++------------
 drivers/s390/char/diag_ftp.c            |  4 +-
 drivers/s390/char/sclp_ftp.c            |  6 +--
 drivers/s390/scsi/zfcp_fc.c             |  8 +--
 drivers/target/target_core_configfs.c   | 33 ++++---------
 drivers/tty/vt/keyboard.c               |  5 +-
 drivers/usb/gadget/function/f_midi.c    |  4 +-
 drivers/usb/gadget/function/f_printer.c |  8 +--
 drivers/usb/usbip/stub_main.c           |  6 +--
 drivers/watchdog/diag288_wdt.c          | 12 +++--
 fs/kernfs/dir.c                         | 27 +++++-----
 kernel/cgroup/cgroup.c                  |  2 +-
 kernel/module.c                         |  4 +-
 kernel/trace/trace_uprobe.c             | 11 ++---
 lib/kobject_uevent.c                    |  6 +--
 net/core/devlink.c                      |  6 +--
 net/sunrpc/clnt.c                       |  6 ++-
 security/integrity/ima/ima_policy.c     |  8 ++-
 sound/usb/card.c                        |  4 +-
 23 files changed, 129 insertions(+), 119 deletions(-)

-- 
2.20.1

[PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 kernel/cgroup/cgroup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1ea995f801ec..bac0dc2ff8ad 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2265,7 +2265,7 @@ int task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)
 		ret = cgroup_path_ns_locked(cgrp, buf, buflen, &init_cgroup_ns);
 	} else {
 		/* if no hierarchy exists, everyone is in "/" */
-		ret = strlcpy(buf, "/", buflen);
+		ret = strscpy(buf, "/", buflen);
 	}
 
 	spin_unlock_irq(&css_set_lock);

Re: [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]

Hello.

On Mon, Feb 22, 2021 at 04:12:12PM +0100, Romain Perier <romain.perier@gmail.com> wrote:
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2265,7 +2265,7 @@ int task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)
Actually, this function isn't used at all. So I'd instead propose the
patch below.

-- >8 --
From 4f7e0b9c0412f60e0b0e8b7d1ef6eb2790dca567 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <mkoutny@suse.com>
Date: Tue, 23 Feb 2021 17:05:57 +0100
Subject: [PATCH] cgroup: Drop task_cgroup_path()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The function has no current users and it is a remnant from kdbus
enthusiasm era 857a2beb09ab ("cgroup: implement
task_cgroup_path_from_hierarchy()"). Drop it to eliminate unused code.

Suggested-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 include/linux/cgroup.h |  1 -
 kernel/cgroup/cgroup.c | 39 ---------------------------------------
 2 files changed, 40 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 4f2f79de083e..e9c41b15fd4e 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -115,7 +115,6 @@ int cgroup_add_legacy_cftypes(struct cgroup_subsys *ss, struct cftype *cfts);
 int cgroup_rm_cftypes(struct cftype *cfts);
 void cgroup_file_notify(struct cgroup_file *cfile);
 
-int task_cgroup_path(struct task_struct *task, char *buf, size_t buflen);
 int cgroupstats_build(struct cgroupstats *stats, struct dentry *dentry);
 int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
 		     struct pid *pid, struct task_struct *tsk);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index c80fe99f85ae..d75ffd461222 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2235,45 +2235,6 @@ int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen,
 }
 EXPORT_SYMBOL_GPL(cgroup_path_ns);
 
-/**
- * task_cgroup_path - cgroup path of a task in the first cgroup hierarchy
- * @task: target task
- * @buf: the buffer to write the path into
- * @buflen: the length of the buffer
- *
- * Determine @task's cgroup on the first (the one with the lowest non-zero
- * hierarchy_id) cgroup hierarchy and copy its path into @buf.  This
- * function grabs cgroup_mutex and shouldn't be used inside locks used by
- * cgroup controller callbacks.
- *
- * Return value is the same as kernfs_path().
- */
-int task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)
-{
-	struct cgroup_root *root;
-	struct cgroup *cgrp;
-	int hierarchy_id = 1;
-	int ret;
-
-	mutex_lock(&cgroup_mutex);
-	spin_lock_irq(&css_set_lock);
-
-	root = idr_get_next(&cgroup_hierarchy_idr, &hierarchy_id);
-
-	if (root) {
-		cgrp = task_cgroup_from_root(task, root);
-		ret = cgroup_path_ns_locked(cgrp, buf, buflen, &init_cgroup_ns);
-	} else {
-		/* if no hierarchy exists, everyone is in "/" */
-		ret = strlcpy(buf, "/", buflen);
-	}
-
-	spin_unlock_irq(&css_set_lock);
-	mutex_unlock(&cgroup_mutex);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(task_cgroup_path);
-
 /**
  * cgroup_migrate_add_task - add a migration target task to a migration context
  * @task: target task
-- 
2.30.1


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 02/20] crypto: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 crypto/lrw.c |    6 +++---
 crypto/xts.c |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/crypto/lrw.c b/crypto/lrw.c
index bcf09fbc750a..4d35f4439012 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -357,10 +357,10 @@ static int lrw_create(struct crypto_template *tmpl, struct rtattr **tb)
 	 * cipher name.
 	 */
 	if (!strncmp(cipher_name, "ecb(", 4)) {
-		unsigned len;
+		ssize_t len;
 
-		len = strlcpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
-		if (len < 2 || len >= sizeof(ecb_name))
+		len = strscpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
+		if (len == -E2BIG || len < 2)
 			goto err_free_inst;
 
 		if (ecb_name[len - 1] != ')')
diff --git a/crypto/xts.c b/crypto/xts.c
index 6c12f30dbdd6..1dfe39d61418 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -396,10 +396,10 @@ static int xts_create(struct crypto_template *tmpl, struct rtattr **tb)
 	 * cipher name.
 	 */
 	if (!strncmp(cipher_name, "ecb(", 4)) {
-		unsigned len;
+		ssize_t len;
 
-		len = strlcpy(ctx->name, cipher_name + 4, sizeof(ctx->name));
-		if (len < 2 || len >= sizeof(ctx->name))
+		len = strscpy(ctx->name, cipher_name + 4, sizeof(ctx->name));
+		if (len == -E2BIG || len < 2)
 			goto err_free_inst;
 
 		if (ctx->name[len - 1] != ')')

Re: [PATCH 02/20] crypto: Manual replacement of the deprecated strlcpy() with return values

[Herbert Xu]

On Mon, Feb 22, 2021 at 04:12:13PM +0100, Romain Perier wrote:
>
> diff --git a/crypto/lrw.c b/crypto/lrw.c
> index bcf09fbc750a..4d35f4439012 100644
> --- a/crypto/lrw.c
> +++ b/crypto/lrw.c
> @@ -357,10 +357,10 @@ static int lrw_create(struct crypto_template *tmpl, struct rtattr **tb)
>  	 * cipher name.
>  	 */
>  	if (!strncmp(cipher_name, "ecb(", 4)) {
> -		unsigned len;
> +		ssize_t len;
>  
> -		len = strlcpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
> -		if (len < 2 || len >= sizeof(ecb_name))
> +		len = strscpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
> +		if (len == -E2BIG || len < 2)

len == -E2BIG is superfluous as len < 2 will catch it anyway.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 03/20] devlink: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 net/core/devlink.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 737b61c2976e..7eb445460c92 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -9461,10 +9461,10 @@ EXPORT_SYMBOL_GPL(devlink_port_param_value_changed);
 void devlink_param_value_str_fill(union devlink_param_value *dst_val,
 				  const char *src)
 {
-	size_t len;
+	ssize_t len;
 
-	len = strlcpy(dst_val->vstr, src, __DEVLINK_PARAM_MAX_STRING_VALUE);
-	WARN_ON(len >= __DEVLINK_PARAM_MAX_STRING_VALUE);
+	len = strscpy(dst_val->vstr, src, __DEVLINK_PARAM_MAX_STRING_VALUE);
+	WARN_ON(len == -E2BIG);
 }
 EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
 

Re: [PATCH 03/20] devlink: Manual replacement of the deprecated strlcpy() with return values

[Jakub Kicinski]

On Mon, 22 Feb 2021 16:12:14 +0100 Romain Perier wrote:
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 737b61c2976e..7eb445460c92 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -9461,10 +9461,10 @@ EXPORT_SYMBOL_GPL(devlink_port_param_value_changed);
>  void devlink_param_value_str_fill(union devlink_param_value *dst_val,
>  				  const char *src)
>  {
> -	size_t len;
> +	ssize_t len;
>  
> -	len = strlcpy(dst_val->vstr, src, __DEVLINK_PARAM_MAX_STRING_VALUE);
> -	WARN_ON(len >= __DEVLINK_PARAM_MAX_STRING_VALUE);
> +	len = strscpy(dst_val->vstr, src, __DEVLINK_PARAM_MAX_STRING_VALUE);
> +	WARN_ON(len == -E2BIG);

WARN_ON(len < 0) ?

[PATCH 04/20] dma-buf: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/dma-buf/dma-buf.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f264b70c383e..515192f2f404 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -42,12 +42,12 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
 {
 	struct dma_buf *dmabuf;
 	char name[DMA_BUF_NAME_LEN];
-	size_t ret = 0;
+	ssize_t ret = 0;
 
 	dmabuf = dentry->d_fsdata;
 	spin_lock(&dmabuf->name_lock);
 	if (dmabuf->name)
-		ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
+		ret = strscpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
 	spin_unlock(&dmabuf->name_lock);
 
 	return dynamic_dname(dentry, buffer, buflen, "/%s:%s",

[PATCH 05/20] kobject: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 lib/kobject_uevent.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7998affa45d4..9dca89b76a22 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -251,11 +251,11 @@ static int kobj_usermode_filter(struct kobject *kobj)
 
 static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem)
 {
-	int len;
+	ssize_t len;
 
-	len = strlcpy(&env->buf[env->buflen], subsystem,
+	len = strscpy(&env->buf[env->buflen], subsystem,
 		      sizeof(env->buf) - env->buflen);
-	if (len >= (sizeof(env->buf) - env->buflen)) {
+	if (len == -E2BIG) {
 		WARN(1, KERN_ERR "init_uevent_argv: buffer size too small\n");
 		return -ENOMEM;
 	}

[PATCH 06/20] ima: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 security/integrity/ima/ima_policy.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9b45d064a87d..1a905b8b064f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -790,8 +790,14 @@ static int __init ima_init_arch_policy(void)
 	for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
 		char rule[255];
 		int result;
+		ssize_t len;
 
-		result = strlcpy(rule, *rules, sizeof(rule));
+		len = strscpy(rule, *rules, sizeof(rule));
+		if (len == -E2BIG) {
+			pr_warn("Internal copy of architecture policy rule '%s' "
+				"failed. Skipping.\n", *rules);
+			continue;
+		}
 
 		INIT_LIST_HEAD(&arch_policy_entry[i].list);
 		result = ima_parse_rule(rule, &arch_policy_entry[i]);

Re: [PATCH 06/20] ima: Manual replacement of the deprecated strlcpy() with return values

[Mimi Zohar]

On Mon, 2021-02-22 at 16:12 +0100, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

As other's have pointed out, "lenght" -> length.

> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  security/integrity/ima/ima_policy.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9b45d064a87d..1a905b8b064f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -790,8 +790,14 @@ static int __init ima_init_arch_policy(void)
>  	for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
>  		char rule[255];
>  		int result;
> +		ssize_t len;
>  
> -		result = strlcpy(rule, *rules, sizeof(rule));
> +		len = strscpy(rule, *rules, sizeof(rule));
> +		if (len == -E2BIG) {
> +			pr_warn("Internal copy of architecture policy rule '%s' "
> +				"failed. Skipping.\n", *rules);

"arch_rules" is an array of hard coded strings.   The generic reason
for replacing strlcpy with strscpy doesn't seem applicable; however,
the additonal warning is appropriate.

(User-visible strings are not bound to the 80 column length.  Breaking
up the line like this is fine, but unnecessary.)

Acked-by: Mimi Zohar <zohar@linux.ibm.com>

thanks,

Mimi

> +			continue;
> +		}
>  
>  		INIT_LIST_HEAD(&arch_policy_entry[i].list);
>  		result = ima_parse_rule(rule, &arch_policy_entry[i]);
> 

[PATCH 07/20] SUNRPC: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 net/sunrpc/clnt.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 612f0a641f4c..3c5c4ad8a808 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -282,7 +282,7 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt,
 
 static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
 {
-	clnt->cl_nodelen = strlcpy(clnt->cl_nodename,
+	clnt->cl_nodelen = strscpy(clnt->cl_nodename,
 			nodename, sizeof(clnt->cl_nodename));
 }
 
@@ -422,6 +422,10 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
 		nodename = utsname()->nodename;
 	/* save the nodename */
 	rpc_clnt_set_nodename(clnt, nodename);
+	if (clnt->cl_nodelen == -E2BIG) {
+		err = -ENOMEM;
+		goto out_no_path;
+	}
 
 	err = rpc_client_register(clnt, args->authflavor, args->client_name);
 	if (err)

Re: [PATCH 07/20] SUNRPC: Manual replacement of the deprecated strlcpy() with return values

[Chuck Lever]

> On Feb 22, 2021, at 10:12 AM, Romain Perier <romain.perier@gmail.com> wrote:
> 
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>

Hi Romain-

I assume you are waiting for a maintainer's Ack? IMHO Trond or Anna
should provide it for changes to this particular source file.


> ---
> net/sunrpc/clnt.c |    6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 612f0a641f4c..3c5c4ad8a808 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -282,7 +282,7 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt,
> 
> static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
> {
> -	clnt->cl_nodelen = strlcpy(clnt->cl_nodename,
> +	clnt->cl_nodelen = strscpy(clnt->cl_nodename,
> 			nodename, sizeof(clnt->cl_nodename));
> }
> 
> @@ -422,6 +422,10 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
> 		nodename = utsname()->nodename;
> 	/* save the nodename */
> 	rpc_clnt_set_nodename(clnt, nodename);
> +	if (clnt->cl_nodelen == -E2BIG) {
> +		err = -ENOMEM;
> +		goto out_no_path;
> +	}
> 
> 	err = rpc_client_register(clnt, args->authflavor, args->client_name);
> 	if (err)
> 

--
Chuck Lever

[PATCH 08/20] kernfs: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 fs/kernfs/dir.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 7a53eed69fef..9e65b595d880 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -42,9 +42,9 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
 static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
 {
 	if (!kn)
-		return strlcpy(buf, "(null)", buflen);
+		return strscpy(buf, "(null)", buflen);
 
-	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
+	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
 }
 
 /* kernfs_node_depth - compute depth from @from to @to */
@@ -125,17 +125,18 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 {
 	struct kernfs_node *kn, *common;
 	const char parent_str[] = "/..";
-	size_t depth_from, depth_to, len = 0;
+	size_t depth_from, depth_to;
+	ssize_t len = 0;
 	int i, j;
 
 	if (!kn_to)
-		return strlcpy(buf, "(null)", buflen);
+		return strscpy(buf, "(null)", buflen);
 
 	if (!kn_from)
 		kn_from = kernfs_root(kn_to)->kn;
 
 	if (kn_from == kn_to)
-		return strlcpy(buf, "/", buflen);
+		return strscpy(buf, "/", buflen);
 
 	if (!buf)
 		return -EINVAL;
@@ -150,16 +151,16 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 	buf[0] = '\0';
 
 	for (i = 0; i < depth_from; i++)
-		len += strlcpy(buf + len, parent_str,
+		len += strscpy(buf + len, parent_str,
 			       len < buflen ? buflen - len : 0);
 
 	/* Calculate how many bytes we need for the rest */
 	for (i = depth_to - 1; i >= 0; i--) {
 		for (kn = kn_to, j = 0; j < i; j++)
 			kn = kn->parent;
-		len += strlcpy(buf + len, "/",
+		len += strscpy(buf + len, "/",
 			       len < buflen ? buflen - len : 0);
-		len += strlcpy(buf + len, kn->name,
+		len += strscpy(buf + len, kn->name,
 			       len < buflen ? buflen - len : 0);
 	}
 
@@ -173,8 +174,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
  * @buflen: size of @buf
  *
  * Copies the name of @kn into @buf of @buflen bytes.  The behavior is
- * similar to strlcpy().  It returns the length of @kn's name and if @buf
- * isn't long enough, it's filled upto @buflen-1 and nul terminated.
+ * similar to strscpy().  It returns the length of @kn's name and if @buf
+ * isn't long enough or @buflen is 0, it returns -E2BIG.
  *
  * Fills buffer with "(null)" if @kn is NULL.
  *
@@ -858,7 +859,7 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
 					  const unsigned char *path,
 					  const void *ns)
 {
-	size_t len;
+	ssize_t len;
 	char *p, *name;
 
 	lockdep_assert_held(&kernfs_mutex);
@@ -866,9 +867,9 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
 	/* grab kernfs_rename_lock to piggy back on kernfs_pr_cont_buf */
 	spin_lock_irq(&kernfs_rename_lock);
 
-	len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
+	len = strscpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
 
-	if (len >= sizeof(kernfs_pr_cont_buf)) {
+	if (len == -E2BIG) {
 		spin_unlock_irq(&kernfs_rename_lock);
 		return NULL;
 	}

[PATCH 09/20] m68k/atari: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/m68k/emu/natfeat.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/m68k/emu/natfeat.c b/arch/m68k/emu/natfeat.c
index 71b78ecee75c..fbb3454d3c6a 100644
--- a/arch/m68k/emu/natfeat.c
+++ b/arch/m68k/emu/natfeat.c
@@ -41,10 +41,10 @@ long nf_get_id(const char *feature_name)
 {
 	/* feature_name may be in vmalloc()ed memory, so make a copy */
 	char name_copy[32];
-	size_t n;
+	ssize_t n;
 
-	n = strlcpy(name_copy, feature_name, sizeof(name_copy));
-	if (n >= sizeof(name_copy))
+	n = strscpy(name_copy, feature_name, sizeof(name_copy));
+	if (n == -E2BIG)
 		return 0;
 
 	return nf_get_id_phys(virt_to_phys(name_copy));

[PATCH 10/20] module: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 kernel/module.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..46aad8e92a81 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2814,6 +2814,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	Elf_Sym *dst;
 	char *s;
 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
+	ssize_t len;
 
 	/* Set up to point into init section. */
 	mod->kallsyms = mod->init_layout.base + info->mod_kallsyms_init_off;
@@ -2841,8 +2842,9 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 			    mod->kallsyms->typetab[i];
 			dst[ndst] = src[i];
 			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
-			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
+			len = strscpy(s, &mod->kallsyms->strtab[src[i].st_name],
 				     KSYM_NAME_LEN) + 1;
+			s += (len != -E2BIG) ? len : 0;
 		}
 	}
 	mod->core_kallsyms.num_symtab = ndst;

[PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/hwmon/pmbus/max20730.c |   66 +++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
index 9dd3dd79bc18..a384b57b7327 100644
--- a/drivers/hwmon/pmbus/max20730.c
+++ b/drivers/hwmon/pmbus/max20730.c
@@ -107,7 +107,8 @@ struct max20730_debugfs_data {
 static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 				     size_t count, loff_t *ppos)
 {
-	int ret, len;
+	int ret;
+	ssize_t len;
 	int *idxp = file->private_data;
 	int idx = *idxp;
 	struct max20730_debugfs_data *psu = to_psu(idxp, idx);
@@ -148,13 +149,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 			>> MAX20730_MFR_DEVSET1_TSTAT_BIT_POS;
 
 		if (val == 0)
-			len = strlcpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
 		else if (val == 1)
-			len = strlcpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
 		else if (val == 2)
-			len = strlcpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
 		else
-			len = strlcpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
 		break;
 	case MAX20730_DEBUGFS_INTERNAL_GAIN:
 		val = (data->mfr_devset1 & MAX20730_MFR_DEVSET1_RGAIN_MASK)
@@ -163,35 +164,35 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 		if (data->id == max20734) {
 			/* AN6209 */
 			if (val == 0)
-				len = strlcpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
 			else if (val == 1)
-				len = strlcpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
 			else if (val == 2)
-				len = strlcpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
 			else
-				len = strlcpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
 		} else if (data->id == max20730 || data->id == max20710) {
 			/* AN6042 or AN6140 */
 			if (val == 0)
-				len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
 			else if (val == 1)
-				len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
 			else if (val == 2)
-				len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
 			else
-				len = strlcpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
 		} else if (data->id == max20743) {
 			/* AN6042 */
 			if (val == 0)
-				len = strlcpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
 			else if (val == 1)
-				len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
 			else if (val == 2)
-				len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
 			else
-				len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
+				len = strscpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
 		} else {
-			len = strlcpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
 		}
 		break;
 	case MAX20730_DEBUGFS_BOOT_VOLTAGE:
@@ -199,26 +200,26 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 			>> MAX20730_MFR_DEVSET1_VBOOT_BIT_POS;
 
 		if (val == 0)
-			len = strlcpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
 		else if (val == 1)
-			len = strlcpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
 		else if (val == 2)
-			len = strlcpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
 		else
-			len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
 		break;
 	case MAX20730_DEBUGFS_OUT_V_RAMP_RATE:
 		val = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_VRATE)
 			>> MAX20730_MFR_DEVSET2_VRATE_BIT_POS;
 
 		if (val == 0)
-			len = strlcpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
 		else if (val == 1)
-			len = strlcpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
 		else if (val == 2)
-			len = strlcpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
 		else
-			len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
 		break;
 	case MAX20730_DEBUGFS_OC_PROTECT_MODE:
 		ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_OCPM_MASK)
@@ -230,13 +231,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 			>> MAX20730_MFR_DEVSET2_SS_BIT_POS;
 
 		if (val == 0)
-			len = strlcpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
 		else if (val == 1)
-			len = strlcpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
 		else if (val == 2)
-			len = strlcpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
 		else
-			len = strlcpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
+			len = strscpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
 		break;
 	case MAX20730_DEBUGFS_IMAX:
 		ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_IMAX_MASK)
@@ -287,9 +288,12 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
 				"%d.%d\n", ret / 10000, ret % 10000);
 		break;
 	default:
-		len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+		len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
 	}
 
+	if (len == -E2BIG)
+		return -E2BIG;
+
 	return simple_read_from_buffer(buf, count, ppos, tbuf, len);
 }
 

Re: [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values

[Guenter Roeck]

On 2/22/21 7:12 AM, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

length

> It can lead to linear read overflows, crashes, etc...
> 
Not here. This description is misleading.

> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>

This patch just adds pain to injury, as the source 'buffers' are all fixed
strings and their length will never exceed the maximum buffer length.
I really don't see the point of using strscpy() in this situation.

> ---
>  drivers/hwmon/pmbus/max20730.c |   66 +++++++++++++++++++++--------------------

This patch only modifies a single driver and should be tagged as such.

>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
> index 9dd3dd79bc18..a384b57b7327 100644
> --- a/drivers/hwmon/pmbus/max20730.c
> +++ b/drivers/hwmon/pmbus/max20730.c
> @@ -107,7 +107,8 @@ struct max20730_debugfs_data {
>  static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  				     size_t count, loff_t *ppos)
>  {
> -	int ret, len;
> +	int ret;
> +	ssize_t len;
>  	int *idxp = file->private_data;
>  	int idx = *idxp;
>  	struct max20730_debugfs_data *psu = to_psu(idxp, idx);
> @@ -148,13 +149,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  			>> MAX20730_MFR_DEVSET1_TSTAT_BIT_POS;
>  
>  		if (val == 0)
> -			len = strlcpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 1)
> -			len = strlcpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 2)
> -			len = strlcpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
>  		else
> -			len = strlcpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
>  		break;
>  	case MAX20730_DEBUGFS_INTERNAL_GAIN:
>  		val = (data->mfr_devset1 & MAX20730_MFR_DEVSET1_RGAIN_MASK)
> @@ -163,35 +164,35 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  		if (data->id == max20734) {
>  			/* AN6209 */
>  			if (val == 0)
> -				len = strlcpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 1)
> -				len = strlcpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 2)
> -				len = strlcpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
>  			else
> -				len = strlcpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
>  		} else if (data->id == max20730 || data->id == max20710) {
>  			/* AN6042 or AN6140 */
>  			if (val == 0)
> -				len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 1)
> -				len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 2)
> -				len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
>  			else
> -				len = strlcpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
>  		} else if (data->id == max20743) {
>  			/* AN6042 */
>  			if (val == 0)
> -				len = strlcpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 1)
> -				len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
>  			else if (val == 2)
> -				len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
>  			else
> -				len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
> +				len = strscpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
>  		} else {
> -			len = strlcpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
>  		}
>  		break;
>  	case MAX20730_DEBUGFS_BOOT_VOLTAGE:
> @@ -199,26 +200,26 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  			>> MAX20730_MFR_DEVSET1_VBOOT_BIT_POS;
>  
>  		if (val == 0)
> -			len = strlcpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 1)
> -			len = strlcpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 2)
> -			len = strlcpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
>  		else
> -			len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
>  		break;
>  	case MAX20730_DEBUGFS_OUT_V_RAMP_RATE:
>  		val = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_VRATE)
>  			>> MAX20730_MFR_DEVSET2_VRATE_BIT_POS;
>  
>  		if (val == 0)
> -			len = strlcpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 1)
> -			len = strlcpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 2)
> -			len = strlcpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
>  		else
> -			len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
>  		break;
>  	case MAX20730_DEBUGFS_OC_PROTECT_MODE:
>  		ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_OCPM_MASK)
> @@ -230,13 +231,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  			>> MAX20730_MFR_DEVSET2_SS_BIT_POS;
>  
>  		if (val == 0)
> -			len = strlcpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 1)
> -			len = strlcpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
>  		else if (val == 2)
> -			len = strlcpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
>  		else
> -			len = strlcpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
> +			len = strscpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
>  		break;
>  	case MAX20730_DEBUGFS_IMAX:
>  		ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_IMAX_MASK)
> @@ -287,9 +288,12 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>  				"%d.%d\n", ret / 10000, ret % 10000);
>  		break;
>  	default:
> -		len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> +		len = strscpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
>  	}
>  
> +	if (len == -E2BIG)
> +		return -E2BIG;
> +
>  	return simple_read_from_buffer(buf, count, ppos, tbuf, len);
>  }
>  
> 

Re: [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values

[Joe Perches]

On Mon, 2021-02-22 at 07:46 -0800, Guenter Roeck wrote:
> On 2/22/21 7:12 AM, Romain Perier wrote:
> > The strlcpy() reads the entire source buffer first, it is dangerous if
> > the source buffer lenght is unbounded or possibility non NULL-terminated.
> 
> length
> 
> > It can lead to linear read overflows, crashes, etc...
> > 
> Not here. This description is misleading.
> 
> > As recommended in the deprecated interfaces [1], it should be replaced
> > by strscpy.
> > 
> > This commit replaces all calls to strlcpy that handle the return values
> > by the corresponding strscpy calls with new handling of the return
> > values (as it is quite different between the two functions).
> > 
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > 
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> 
> This patch just adds pain to injury, as the source 'buffers' are all fixed
> strings and their length will never exceed the maximum buffer length.
> I really don't see the point of using strscpy() in this situation.

Might as well just use strcpy (I'd still prefer stracpy)

https://lore.kernel.org/lkml/24bb53c57767c1c2a8f266c305a670f7@sk2.org/T/

[PATCH 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/s390/char/diag_ftp.c |    4 ++--
 drivers/s390/char/sclp_ftp.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/char/diag_ftp.c b/drivers/s390/char/diag_ftp.c
index 6bf1058de873..c198dfcc85be 100644
--- a/drivers/s390/char/diag_ftp.c
+++ b/drivers/s390/char/diag_ftp.c
@@ -158,8 +158,8 @@ ssize_t diag_ftp_cmd(const struct hmcdrv_ftp_cmdspec *ftp, size_t *fsize)
 		goto out;
 	}
 
-	len = strlcpy(ldfpl->fident, ftp->fname, sizeof(ldfpl->fident));
-	if (len >= HMCDRV_FTP_FIDENT_MAX) {
+	len = strscpy(ldfpl->fident, ftp->fname, sizeof(ldfpl->fident));
+	if (len == -E2BIG) {
 		len = -EINVAL;
 		goto out_free;
 	}
diff --git a/drivers/s390/char/sclp_ftp.c b/drivers/s390/char/sclp_ftp.c
index dfdd6c8fd17e..525156926592 100644
--- a/drivers/s390/char/sclp_ftp.c
+++ b/drivers/s390/char/sclp_ftp.c
@@ -87,7 +87,7 @@ static int sclp_ftp_et7(const struct hmcdrv_ftp_cmdspec *ftp)
 	struct completion completion;
 	struct sclp_diag_sccb *sccb;
 	struct sclp_req *req;
-	size_t len;
+	ssize_t len;
 	int rc;
 
 	req = kzalloc(sizeof(*req), GFP_KERNEL);
@@ -114,9 +114,9 @@ static int sclp_ftp_et7(const struct hmcdrv_ftp_cmdspec *ftp)
 	sccb->evbuf.mdd.ftp.length = ftp->len;
 	sccb->evbuf.mdd.ftp.bufaddr = virt_to_phys(ftp->buf);
 
-	len = strlcpy(sccb->evbuf.mdd.ftp.fident, ftp->fname,
+	len = strscpy(sccb->evbuf.mdd.ftp.fident, ftp->fname,
 		      HMCDRV_FTP_FIDENT_MAX);
-	if (len >= HMCDRV_FTP_FIDENT_MAX) {
+	if (len == -E2BIG) {
 		rc = -EINVAL;
 		goto out_free;
 	}

[PATCH 13/20] scsi: zfcp: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/s390/scsi/zfcp_fc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
index d24cafe02708..8a65241011b9 100644
--- a/drivers/s390/scsi/zfcp_fc.c
+++ b/drivers/s390/scsi/zfcp_fc.c
@@ -877,14 +877,16 @@ static void zfcp_fc_rspn(struct zfcp_adapter *adapter,
 	struct zfcp_fsf_ct_els *ct_els = &fc_req->ct_els;
 	struct zfcp_fc_rspn_req *rspn_req = &fc_req->u.rspn.req;
 	struct fc_ct_hdr *rspn_rsp = &fc_req->u.rspn.rsp;
-	int ret, len;
+	int ret;
+	ssize_t len;
 
 	zfcp_fc_ct_ns_init(&rspn_req->ct_hdr, FC_NS_RSPN_ID,
 			   FC_SYMBOLIC_NAME_SIZE);
 	hton24(rspn_req->rspn.fr_fid.fp_fid, fc_host_port_id(shost));
-	len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
+	len = strscpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
 		      FC_SYMBOLIC_NAME_SIZE);
-	rspn_req->rspn.fr_name_len = len;
+	if (len != -E2BIG)
+		rspn_req->rspn.fr_name_len = len;
 
 	sg_init_one(&fc_req->sg_req, rspn_req, sizeof(*rspn_req));
 	sg_init_one(&fc_req->sg_rsp, rspn_rsp, sizeof(*rspn_rsp));

Re: [PATCH 13/20] scsi: zfcp: Manual replacement of the deprecated strlcpy() with return values

[Benjamin Block]

On Mon, Feb 22, 2021 at 04:12:24PM +0100, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  drivers/s390/scsi/zfcp_fc.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> index d24cafe02708..8a65241011b9 100644
> --- a/drivers/s390/scsi/zfcp_fc.c
> +++ b/drivers/s390/scsi/zfcp_fc.c
> @@ -877,14 +877,16 @@ static void zfcp_fc_rspn(struct zfcp_adapter *adapter,
>  	struct zfcp_fsf_ct_els *ct_els = &fc_req->ct_els;
>  	struct zfcp_fc_rspn_req *rspn_req = &fc_req->u.rspn.req;
>  	struct fc_ct_hdr *rspn_rsp = &fc_req->u.rspn.rsp;
> -	int ret, len;
> +	int ret;
> +	ssize_t len;
>  
>  	zfcp_fc_ct_ns_init(&rspn_req->ct_hdr, FC_NS_RSPN_ID,
>  			   FC_SYMBOLIC_NAME_SIZE);
>  	hton24(rspn_req->rspn.fr_fid.fp_fid, fc_host_port_id(shost));
> -	len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
> +	len = strscpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost),
>  		      FC_SYMBOLIC_NAME_SIZE);
> -	rspn_req->rspn.fr_name_len = len;
> +	if (len != -E2BIG)
> +		rspn_req->rspn.fr_name_len = len;

That is a bug. Leaving `rspn.fr_name_len` uninitialized defeats the
purpose of sending a RSPN.

How about:
	if (len == -E2BIG)
		rspn_req->rspn.fr_name_len = FC_SYMBOLIC_NAME_SIZE - 1;
	else
		rspn_req->rspn.fr_name_len = len;

>  
>  	sg_init_one(&fc_req->sg_req, rspn_req, sizeof(*rspn_req));
>  	sg_init_one(&fc_req->sg_rsp, rspn_rsp, sizeof(*rspn_rsp));
> 

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

[PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/target/target_core_configfs.c |   33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index f04352285155..676215cd8847 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1325,16 +1325,11 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_VENDOR_LEN + 2];
 	char *stripped = NULL;
-	size_t len;
+	ssize_t len;
 	ssize_t ret;
 
-	len = strlcpy(buf, page, sizeof(buf));
-	if (len < sizeof(buf)) {
-		/* Strip any newline added from userspace. */
-		stripped = strstrip(buf);
-		len = strlen(stripped);
-	}
-	if (len > INQUIRY_VENDOR_LEN) {
+	len = strscpy(buf, page, sizeof(buf));
+	if (len == -E2BIG) {
 		pr_err("Emulated T10 Vendor Identification exceeds"
 			" INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
 			"\n");
@@ -1381,16 +1376,11 @@ static ssize_t target_wwn_product_id_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_MODEL_LEN + 2];
 	char *stripped = NULL;
-	size_t len;
+	ssize_t len;
 	ssize_t ret;
 
-	len = strlcpy(buf, page, sizeof(buf));
-	if (len < sizeof(buf)) {
-		/* Strip any newline added from userspace. */
-		stripped = strstrip(buf);
-		len = strlen(stripped);
-	}
-	if (len > INQUIRY_MODEL_LEN) {
+	len = strscpy(buf, page, sizeof(buf));
+	if (len == -E2BIG) {
 		pr_err("Emulated T10 Vendor exceeds INQUIRY_MODEL_LEN: "
 			 __stringify(INQUIRY_MODEL_LEN)
 			"\n");
@@ -1437,16 +1427,11 @@ static ssize_t target_wwn_revision_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_REVISION_LEN + 2];
 	char *stripped = NULL;
-	size_t len;
+	ssize_t len;
 	ssize_t ret;
 
-	len = strlcpy(buf, page, sizeof(buf));
-	if (len < sizeof(buf)) {
-		/* Strip any newline added from userspace. */
-		stripped = strstrip(buf);
-		len = strlen(stripped);
-	}
-	if (len > INQUIRY_REVISION_LEN) {
+	len = strscpy(buf, page, sizeof(buf));
+	if (len == -E2BIG) {
 		pr_err("Emulated T10 Revision exceeds INQUIRY_REVISION_LEN: "
 			 __stringify(INQUIRY_REVISION_LEN)
 			"\n");

Re: [PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values

[Bodo Stroesser]

On 22.02.21 16:12, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>   drivers/target/target_core_configfs.c |   33 +++++++++------------------------
>   1 file changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index f04352285155..676215cd8847 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1325,16 +1325,11 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item,
>   	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>   	unsigned char buf[INQUIRY_VENDOR_LEN + 2];
>   	char *stripped = NULL;
> -	size_t len;
> +	ssize_t len;
>   	ssize_t ret;
>   
> -	len = strlcpy(buf, page, sizeof(buf));
> -	if (len < sizeof(buf)) {
> -		/* Strip any newline added from userspace. */
> -		stripped = strstrip(buf);
> -		len = strlen(stripped);
> -	}
> -	if (len > INQUIRY_VENDOR_LEN) {
> +	len = strscpy(buf, page, sizeof(buf));
> +	if (len == -E2BIG) {
>   		pr_err("Emulated T10 Vendor Identification exceeds"
>   			" INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
>   			"\n");
> @@ -1381,16 +1376,11 @@ static ssize_t target_wwn_product_id_store(struct config_item *item,
>   	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>   	unsigned char buf[INQUIRY_MODEL_LEN + 2];
>   	char *stripped = NULL;
> -	size_t len;
> +	ssize_t len;
>   	ssize_t ret;
>   
> -	len = strlcpy(buf, page, sizeof(buf));
> -	if (len < sizeof(buf)) {
> -		/* Strip any newline added from userspace. */
> -		stripped = strstrip(buf);
> -		len = strlen(stripped);
> -	}
> -	if (len > INQUIRY_MODEL_LEN) {
> +	len = strscpy(buf, page, sizeof(buf));
> +	if (len == -E2BIG) {
>   		pr_err("Emulated T10 Vendor exceeds INQUIRY_MODEL_LEN: "
>   			 __stringify(INQUIRY_MODEL_LEN)
>   			"\n");
> @@ -1437,16 +1427,11 @@ static ssize_t target_wwn_revision_store(struct config_item *item,
>   	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>   	unsigned char buf[INQUIRY_REVISION_LEN + 2];
>   	char *stripped = NULL;
> -	size_t len;
> +	ssize_t len;
>   	ssize_t ret;
>   
> -	len = strlcpy(buf, page, sizeof(buf));
> -	if (len < sizeof(buf)) {
> -		/* Strip any newline added from userspace. */
> -		stripped = strstrip(buf);
> -		len = strlen(stripped);
> -	}
> -	if (len > INQUIRY_REVISION_LEN) {
> +	len = strscpy(buf, page, sizeof(buf));
> +	if (len == -E2BIG) {
>   		pr_err("Emulated T10 Revision exceeds INQUIRY_REVISION_LEN: "
>   			 __stringify(INQUIRY_REVISION_LEN)
>   			"\n");
> 

AFAICS, you are not only replacing strlcpy with strscpy, but also remove 
stripping of possible trailing '\n', and remove the necessary length
check of the remaining string.

-Bodo

Re: [PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4477 bytes --]

Hi Romain,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on s390/features]
[also build test WARNING on cryptodev/master crypto/master driver-core/driver-core-testing linus/master v5.11 next-20210222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Romain-Perier/Manual-replacement-of-all-strlcpy-in-favor-of-strscpy/20210222-232701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: x86_64-randconfig-s022-20210222 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-229-g60c1f270-dirty
        # https://github.com/0day-ci/linux/commit/8b76bc6bcdb6b3d4847d4c6298b53759acc0849a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Romain-Perier/Manual-replacement-of-all-strlcpy-in-favor-of-strscpy/20210222-232701
        git checkout 8b76bc6bcdb6b3d4847d4c6298b53759acc0849a
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/target/target_core_configfs.c: In function 'target_check_inquiry_data.constprop':
>> drivers/target/target_core_configfs.c:1293:8: warning: argument 1 null where non-null expected [-Wnonnull]
    1293 |  len = strlen(buf);
         |        ^~~~~~~~~~~
   In file included from include/linux/bitmap.h:9,
                    from include/linux/cpumask.h:12,
                    from arch/x86/include/asm/cpumask.h:5,
                    from arch/x86/include/asm/msr.h:11,
                    from arch/x86/include/asm/processor.h:22,
                    from arch/x86/include/asm/timex.h:5,
                    from include/linux/timex.h:65,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/target/target_core_configfs.c:15:
   include/linux/string.h:89:24: note: in a call to function 'strlen' declared here
      89 | extern __kernel_size_t strlen(const char *);
         |                        ^~~~~~


vim +1293 drivers/target/target_core_configfs.c

c66ac9db8d4ad9 Nicholas Bellinger 2010-12-17  1287  
0322913cab79e4 Alan Adamson       2019-03-01  1288  static ssize_t target_check_inquiry_data(char *buf)
0322913cab79e4 Alan Adamson       2019-03-01  1289  {
0322913cab79e4 Alan Adamson       2019-03-01  1290  	size_t len;
0322913cab79e4 Alan Adamson       2019-03-01  1291  	int i;
0322913cab79e4 Alan Adamson       2019-03-01  1292  
0322913cab79e4 Alan Adamson       2019-03-01 @1293  	len = strlen(buf);
0322913cab79e4 Alan Adamson       2019-03-01  1294  
0322913cab79e4 Alan Adamson       2019-03-01  1295  	/*
0322913cab79e4 Alan Adamson       2019-03-01  1296  	 * SPC 4.3.1:
0322913cab79e4 Alan Adamson       2019-03-01  1297  	 * ASCII data fields shall contain only ASCII printable characters
0322913cab79e4 Alan Adamson       2019-03-01  1298  	 * (i.e., code values 20h to 7Eh) and may be terminated with one or
0322913cab79e4 Alan Adamson       2019-03-01  1299  	 * more ASCII null (00h) characters.
0322913cab79e4 Alan Adamson       2019-03-01  1300  	 */
0322913cab79e4 Alan Adamson       2019-03-01  1301  	for (i = 0; i < len; i++) {
0322913cab79e4 Alan Adamson       2019-03-01  1302  		if (buf[i] < 0x20 || buf[i] > 0x7E) {
0322913cab79e4 Alan Adamson       2019-03-01  1303  			pr_err("Emulated T10 Inquiry Data contains non-ASCII-printable characters\n");
0322913cab79e4 Alan Adamson       2019-03-01  1304  			return -EINVAL;
0322913cab79e4 Alan Adamson       2019-03-01  1305  		}
0322913cab79e4 Alan Adamson       2019-03-01  1306  	}
0322913cab79e4 Alan Adamson       2019-03-01  1307  
0322913cab79e4 Alan Adamson       2019-03-01  1308  	return len;
0322913cab79e4 Alan Adamson       2019-03-01  1309  }
0322913cab79e4 Alan Adamson       2019-03-01  1310  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41881 bytes --]

[PATCH 15/20] ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 sound/usb/card.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 85ed8507e41a..acb1ea3e16a3 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -496,7 +496,7 @@ static void usb_audio_make_longname(struct usb_device *dev,
 	struct snd_card *card = chip->card;
 	const struct usb_audio_device_name *preset;
 	const char *s = NULL;
-	int len;
+	ssize_t len;
 
 	preset = lookup_device_name(chip->usb_id);
 
-- 
2.20.1

Re: [PATCH 15/20] ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with return values

[Takashi Iwai]

On Mon, 22 Feb 2021 16:12:26 +0100,
Romain Perier wrote:
> 
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>

The strlcpy() usage in sound/* have been already converted on the
latest Linus tree.  So please drop this one.


thanks,

Takashi

[PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 kernel/trace/trace_uprobe.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3cf7128e1ad3..f9583afdb735 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -154,12 +154,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
 	u8 *dst = get_loc_data(dest, base);
 	void __user *src = (void __force __user *) addr;
 
-	if (unlikely(!maxlen))
-		return -ENOMEM;
-
-	if (addr == FETCH_TOKEN_COMM)
-		ret = strlcpy(dst, current->comm, maxlen);
-	else
+	if (addr == FETCH_TOKEN_COMM) {
+		ret = strscpy(dst, current->comm, maxlen);
+		if (ret == -E2BIG)
+			return -ENOMEM;
+	} else
 		ret = strncpy_from_user(dst, src, maxlen);
 	if (ret >= 0) {
 		if (ret == maxlen)

Re: [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values

[Steven Rostedt]

On Mon, 22 Feb 2021 16:12:27 +0100
Romain Perier <romain.perier@gmail.com> wrote:

> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  kernel/trace/trace_uprobe.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 3cf7128e1ad3..f9583afdb735 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -154,12 +154,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	u8 *dst = get_loc_data(dest, base);
>  	void __user *src = (void __force __user *) addr;
>  
> -	if (unlikely(!maxlen))
> -		return -ENOMEM;

Don't remove the above. You just broke the else side.

> -
> -	if (addr == FETCH_TOKEN_COMM)
> -		ret = strlcpy(dst, current->comm, maxlen);
> -	else
> +	if (addr == FETCH_TOKEN_COMM) {
> +		ret = strscpy(dst, current->comm, maxlen);
> +		if (ret == -E2BIG)
> +			return -ENOMEM;

I'm not sure the above is what we want. current->comm is always nul
terminated, and not only that, it will never be bigger than TASK_COMM_LEN.
If the "dst" location is smaller than comm (maxlen < TASK_COMM_LEN), it is
still OK to copy a partial string. It should not return -ENOMEM which looks
to be what happens with this patch.

In other words, it looks like this patch breaks the current code in more
ways than one.

-- Steve


> +	} else
>  		ret = strncpy_from_user(dst, src, maxlen);
>  	if (ret >= 0) {
>  		if (ret == maxlen)

[PATCH 17/20] vt: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/tty/vt/keyboard.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 77638629c562..5e20c6c307e0 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2067,9 +2067,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 			return -ENOMEM;
 
 		spin_lock_irqsave(&func_buf_lock, flags);
-		len = strlcpy(kbs, func_table[kb_func] ? : "", len);
+		len = strscpy(kbs, func_table[kb_func] ? : "", len);
 		spin_unlock_irqrestore(&func_buf_lock, flags);
 
+		if (len == -E2BIG)
+			return -E2BIG;
+
 		ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
 			-EFAULT : 0;
 

Re: [PATCH 17/20] vt: Manual replacement of the deprecated strlcpy() with return values

[Jiri Slaby]

On 22. 02. 21, 16:12, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

"length" and it's NUL, not NULL in this case.

> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values

s/that/which/ ?
"handles"
"value"

> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).

Sorry, I have hard times understand the whole sentence. Could you 
rephrase a bit?

> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>   drivers/tty/vt/keyboard.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 77638629c562..5e20c6c307e0 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2067,9 +2067,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>   			return -ENOMEM;
>   
>   		spin_lock_irqsave(&func_buf_lock, flags);
> -		len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> +		len = strscpy(kbs, func_table[kb_func] ? : "", len);

func_table[kb_func] is NUL-terminated and kbs is of length len anyway, 
so this is only cosmetical.

>   		spin_unlock_irqrestore(&func_buf_lock, flags);
>   
> +		if (len == -E2BIG)
> +			return -E2BIG;
> +

This can never happen, right?

>   		ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
>   			-EFAULT : 0;
>   
> 

thanks,
-- 
js

[PATCH 18/20] usb: gadget: f_midi: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/usb/gadget/function/f_midi.c    |    4 ++--
 drivers/usb/gadget/function/f_printer.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 71a1a26e85c7..1f2b0d4309b4 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1143,11 +1143,11 @@ F_MIDI_OPT(out_ports, true, MAX_PORTS);
 static ssize_t f_midi_opts_id_show(struct config_item *item, char *page)
 {
 	struct f_midi_opts *opts = to_f_midi_opts(item);
-	int result;
+	ssize_t result;
 
 	mutex_lock(&opts->lock);
 	if (opts->id) {
-		result = strlcpy(page, opts->id, PAGE_SIZE);
+		result = strscpy(page, opts->id, PAGE_SIZE);
 	} else {
 		page[0] = 0;
 		result = 0;
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 61ce8e68f7a3..af83953e6770 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1212,15 +1212,15 @@ static ssize_t f_printer_opts_pnp_string_show(struct config_item *item,
 					      char *page)
 {
 	struct f_printer_opts *opts = to_f_printer_opts(item);
-	int result = 0;
+	ssize_t result = 0;
 
 	mutex_lock(&opts->lock);
 	if (!opts->pnp_string)
 		goto unlock;
 
-	result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
-	if (result >= PAGE_SIZE) {
-		result = PAGE_SIZE;
+	result = strscpy(page, opts->pnp_string, PAGE_SIZE);
+	if (result == -E2BIG) {
+		goto unlock;
 	} else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
 		page[result++] = '\n';
 		page[result] = '\0';

[PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/usb/usbip/stub_main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 77a5b3f8736a..5bc2c09c0d10 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -167,15 +167,15 @@ static ssize_t match_busid_show(struct device_driver *drv, char *buf)
 static ssize_t match_busid_store(struct device_driver *dev, const char *buf,
 				 size_t count)
 {
-	int len;
+	ssize_t len;
 	char busid[BUSID_SIZE];
 
 	if (count < 5)
 		return -EINVAL;
 
 	/* busid needs to include \0 termination */
-	len = strlcpy(busid, buf + 4, BUSID_SIZE);
-	if (sizeof(busid) <= len)
+	len = strscpy(busid, buf + 4, BUSID_SIZE);
+	if (len == -E2BIG)
 		return -EINVAL;
 
 	if (!strncmp(buf, "add ", 4)) {

Re: [PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values

[Shuah Khan]

On 2/22/21 8:12 AM, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>   drivers/usb/usbip/stub_main.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 77a5b3f8736a..5bc2c09c0d10 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -167,15 +167,15 @@ static ssize_t match_busid_show(struct device_driver *drv, char *buf)
>   static ssize_t match_busid_store(struct device_driver *dev, const char *buf,
>   				 size_t count)
>   {
> -	int len;
> +	ssize_t len;
>   	char busid[BUSID_SIZE];
>   
>   	if (count < 5)
>   		return -EINVAL;
>   
>   	/* busid needs to include \0 termination */
> -	len = strlcpy(busid, buf + 4, BUSID_SIZE);
> -	if (sizeof(busid) <= len)
> +	len = strscpy(busid, buf + 4, BUSID_SIZE);
> +	if (len == -E2BIG)
>   		return -EINVAL;
>   
>   	if (!strncmp(buf, "add ", 4)) {
> 


Looks good to me. Thank you.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values

[Sergei Shtylyov]

Hello!

On 22.02.2021 18:12, Romain Perier wrote:

> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

    Length. Possibly?

> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
[...]

MBR, Sergei

[PATCH 20/20] s390/watchdog: Manual replacement of the deprecated strlcpy() with return values

[Romain Perier]

The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...

As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.

This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/watchdog/diag288_wdt.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index aafc8d98bf9f..5703f35dd0b7 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -111,7 +111,7 @@ static unsigned long wdt_status;
 static int wdt_start(struct watchdog_device *dev)
 {
 	char *ebc_cmd;
-	size_t len;
+	ssize_t len;
 	int ret;
 	unsigned int func;
 
@@ -126,7 +126,9 @@ static int wdt_start(struct watchdog_device *dev)
 			clear_bit(DIAG_WDOG_BUSY, &wdt_status);
 			return -ENOMEM;
 		}
-		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
+		len = strscpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
+		if (len == -E2BIG)
+			return -E2BIG;
 		ASCEBC(ebc_cmd, MAX_CMDLEN);
 		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
 
@@ -163,7 +165,7 @@ static int wdt_stop(struct watchdog_device *dev)
 static int wdt_ping(struct watchdog_device *dev)
 {
 	char *ebc_cmd;
-	size_t len;
+	ssize_t len;
 	int ret;
 	unsigned int func;
 
@@ -173,7 +175,9 @@ static int wdt_ping(struct watchdog_device *dev)
 		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
 		if (!ebc_cmd)
 			return -ENOMEM;
-		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
+		len = strscpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
+		if (len == -E2BIG)
+			return -E2BIG;
 		ASCEBC(ebc_cmd, MAX_CMDLEN);
 		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
 

Re: [PATCH 20/20] s390/watchdog: Manual replacement of the deprecated strlcpy() with return values

[Guenter Roeck]

On 2/22/21 7:12 AM, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

length

> It can lead to linear read overflows, crashes, etc...
> 

That won't happen here since both buffers have the same length
and the source is known to be NULL_terminated.

So this is another misleading patch. If strlcpy() is deemed so dangerous
nowadays, replace it with memcpy(). That is expensive too, but at least
it won't create the impression that a non-existing error would ever occur
On top of that, the code is handling all MAX_CMDLEN bytes twice anyway
later on, so we can for sure afford the extra cost of the memcpy().
Pus, we'll have the added benefit that the unused part of the buffer
will be cleared and no longer contain random data.

Guenter

> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  drivers/watchdog/diag288_wdt.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index aafc8d98bf9f..5703f35dd0b7 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -111,7 +111,7 @@ static unsigned long wdt_status;
>  static int wdt_start(struct watchdog_device *dev)
>  {
>  	char *ebc_cmd;
> -	size_t len;
> +	ssize_t len;
>  	int ret;
>  	unsigned int func;
>  
> @@ -126,7 +126,9 @@ static int wdt_start(struct watchdog_device *dev)
>  			clear_bit(DIAG_WDOG_BUSY, &wdt_status);
>  			return -ENOMEM;
>  		}
> -		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> +		len = strscpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> +		if (len == -E2BIG)
> +			return -E2BIG;
>  		ASCEBC(ebc_cmd, MAX_CMDLEN);
>  		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
>  
> @@ -163,7 +165,7 @@ static int wdt_stop(struct watchdog_device *dev)
>  static int wdt_ping(struct watchdog_device *dev)
>  {
>  	char *ebc_cmd;
> -	size_t len;
> +	ssize_t len;
>  	int ret;
>  	unsigned int func;
>  
> @@ -173,7 +175,9 @@ static int wdt_ping(struct watchdog_device *dev)
>  		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
>  		if (!ebc_cmd)
>  			return -ENOMEM;
> -		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> +		len = strscpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> +		if (len == -E2BIG)
> +			return -E2BIG;
>  		ASCEBC(ebc_cmd, MAX_CMDLEN);
>  		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
>  
> 

Re: [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy

[Shuah Khan]

On 2/22/21 8:12 AM, Romain Perier wrote:
> strlcpy() copy a C-String into a sized buffer, the result is always a
> valid NULL-terminated that fits in the buffer, howerver it has severals
> issues. It reads the source buffer first, which is dangerous if it is non
> NULL-terminated or if the corresponding buffer is unbounded. Its safe
> replacement is strscpy(), as suggested in the deprecated interface [1].
> 
> We plan to make this contribution in two steps:
> - Firsly all cases of strlcpy's return value are manually replaced by the
>    corresponding calls of strscpy() with the new handling of the return
>    value (as the return code is different in case of error).
> - Then all other cases are automatically replaced by using coccinelle.
> 

Cool. A quick check shows me 1031 strscpy() calls with no return
checks. All or some of these probably need to be reviewed and add
return checks. Is this something that is in the plan to address as
part of this work?

thanks,
-- Shuah