linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy
@ 2021-02-22 15:12 Romain Perier
  2021-02-22 15:12 ` [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values Romain Perier
                   ` (20 more replies)
  0 siblings, 21 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Tejun Heo, Zefan Li,
	Johannes Weiner, Herbert Xu, David S. Miller, Jiri Pirko,
	Sumit Semwal, Christian König, Greg Kroah-Hartman,
	Mimi Zohar, Dmitry Kasatkin, J. Bruce Fields, Chuck Lever,
	Geert Uytterhoeven, Jessica Yu, Guenter Roeck, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Steffen Maier,
	Benjamin Block, Martin K. Petersen, Jaroslav Kysela,
	Takashi Iwai, Steven Rostedt, Ingo Molnar, Jiri Slaby,
	Felipe Balbi, Valentina Manea, Shuah Khan, Shuah Khan,
	Wim Van Sebroeck
  Cc: Romain Perier, cgroups, linux-crypto, netdev, linux-media,
	dri-devel, linaro-mm-sig, Rafael J. Wysocki, linux-integrity,
	linux-nfs, linux-m68k, linux-hwmon, linux-s390, linux-scsi,
	target-devel, alsa-devel, linux-usb, linux-watchdog,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-23 16:13   ` Michal Koutný
  2021-02-22 15:12 ` [PATCH 02/20] crypto: " Romain Perier
                   ` (19 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Tejun Heo, Zefan Li, Johannes Weiner
  Cc: Romain Perier, cgroups, linux-kernel

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);


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 02/20] crypto: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
  2021-02-22 15:12 ` [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-03-04  4:37   ` Herbert Xu
  2021-02-22 15:12 ` [PATCH 03/20] devlink: " Romain Perier
                   ` (18 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Herbert Xu, David S. Miller
  Cc: Romain Perier, linux-crypto, linux-kernel

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] != ')')


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 03/20] devlink: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
  2021-02-22 15:12 ` [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values Romain Perier
  2021-02-22 15:12 ` [PATCH 02/20] crypto: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-23  0:56   ` Jakub Kicinski
  2021-02-22 15:12 ` [PATCH 04/20] dma-buf: " Romain Perier
                   ` (17 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Jiri Pirko
  Cc: Romain Perier, netdev, linux-kernel

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);
 


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 04/20] dma-buf: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (2 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 03/20] devlink: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 05/20] kobject: " Romain Perier
                   ` (16 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Sumit Semwal, Christian König
  Cc: Romain Perier, linux-media, dri-devel, linaro-mm-sig, linux-kernel

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",


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 05/20] kobject: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (3 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 04/20] dma-buf: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 06/20] ima: " Romain Perier
                   ` (15 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Greg Kroah-Hartman
  Cc: Romain Perier, Rafael J. Wysocki, linux-kernel

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;
 	}


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 06/20] ima: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (4 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 05/20] kobject: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-03-02 13:29   ` Mimi Zohar
  2021-02-22 15:12 ` [PATCH 07/20] SUNRPC: " Romain Perier
                   ` (14 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Mimi Zohar, Dmitry Kasatkin
  Cc: Romain Perier, linux-integrity, linux-kernel

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]);


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 07/20] SUNRPC: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (5 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 06/20] ima: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-03-01 18:25   ` Chuck Lever
  2021-02-22 15:12 ` [PATCH 08/20] kernfs: " Romain Perier
                   ` (13 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, J. Bruce Fields, Chuck Lever
  Cc: Romain Perier, linux-nfs, linux-kernel

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)


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 08/20] kernfs: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (6 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 07/20] SUNRPC: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 09/20] m68k/atari: " Romain Perier
                   ` (12 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Greg Kroah-Hartman, Tejun Heo
  Cc: Romain Perier, linux-kernel

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;
 	}


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 09/20] m68k/atari: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (7 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 08/20] kernfs: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 10/20] module: " Romain Perier
                   ` (11 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Geert Uytterhoeven
  Cc: Romain Perier, linux-m68k, linux-kernel

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));


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 10/20] module: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (8 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 09/20] m68k/atari: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 11/20] hwmon: " Romain Perier
                   ` (10 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Jessica Yu; +Cc: Romain Perier, linux-kernel

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;


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (9 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 10/20] module: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:46   ` Guenter Roeck
  2021-02-22 15:12 ` [PATCH 12/20] s390/hmcdrv: " Romain Perier
                   ` (9 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Guenter Roeck
  Cc: Romain Perier, linux-hwmon, linux-kernel

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);
 }
 


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (10 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 11/20] hwmon: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
                   ` (8 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger
  Cc: Romain Perier, linux-s390, linux-kernel

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;
 	}


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 13/20] scsi: zfcp: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (11 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 12/20] s390/hmcdrv: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 16:04   ` Benjamin Block
  2021-02-22 15:12 ` [PATCH 14/20] target: " Romain Perier
                   ` (7 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Steffen Maier, Benjamin Block
  Cc: Romain Perier, linux-s390, linux-kernel

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));


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (12 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 16:00   ` Bodo Stroesser
  2021-02-22 18:09   ` kernel test robot
  2021-02-22 15:12 ` [PATCH 15/20] ALSA: usb-audio: " Romain Perier
                   ` (6 subsequent siblings)
  20 siblings, 2 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Martin K. Petersen
  Cc: Romain Perier, linux-scsi, target-devel, linux-kernel

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");


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 15/20] ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (13 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 14/20] target: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:39   ` Takashi Iwai
  2021-02-22 15:12 ` [PATCH 16/20] tracing/probe: " Romain Perier
                   ` (5 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Jaroslav Kysela, Takashi Iwai
  Cc: Romain Perier, alsa-devel, linux-kernel

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



^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (14 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 15/20] ALSA: usb-audio: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 17:49   ` Steven Rostedt
  2021-02-22 15:12 ` [PATCH 17/20] vt: " Romain Perier
                   ` (4 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Steven Rostedt, Ingo Molnar
  Cc: Romain Perier, linux-kernel

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)


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 17/20] vt: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (15 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 16/20] tracing/probe: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-26  9:49   ` Jiri Slaby
  2021-02-22 15:12 ` [PATCH 18/20] usb: gadget: f_midi: " Romain Perier
                   ` (3 subsequent siblings)
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Greg Kroah-Hartman, Jiri Slaby
  Cc: Romain Perier, linux-kernel

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;
 


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 18/20] usb: gadget: f_midi: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (16 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 17/20] vt: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:12 ` [PATCH 19/20] usbip: usbip_host: " Romain Perier
                   ` (2 subsequent siblings)
  20 siblings, 0 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Greg Kroah-Hartman, Felipe Balbi
  Cc: Romain Perier, linux-usb, linux-kernel

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';


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (17 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 18/20] usb: gadget: f_midi: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 16:21   ` Shuah Khan
  2021-02-28  9:03   ` Sergei Shtylyov
  2021-02-22 15:12 ` [PATCH 20/20] s390/watchdog: " Romain Perier
  2021-02-22 16:36 ` [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Shuah Khan
  20 siblings, 2 replies; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Greg Kroah-Hartman, Valentina Manea,
	Shuah Khan, Shuah Khan
  Cc: Romain Perier, linux-usb, linux-kernel

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)) {


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 20/20] s390/watchdog: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (18 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 19/20] usbip: usbip_host: " Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
  2021-02-22 15:55   ` Guenter Roeck
  2021-02-22 16:36 ` [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Shuah Khan
  20 siblings, 1 reply; 38+ messages in thread
From: Romain Perier @ 2021-02-22 15:12 UTC (permalink / raw)
  To: Kees Cook, kernel-hardening, Wim Van Sebroeck, Guenter Roeck
  Cc: Romain Perier, linux-watchdog, linux-kernel

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);
 


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 15/20] ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 15/20] ALSA: usb-audio: " Romain Perier
@ 2021-02-22 15:39   ` Takashi Iwai
  0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-02-22 15:39 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, Jaroslav Kysela, Takashi Iwai,
	alsa-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 11/20] hwmon: " Romain Perier
@ 2021-02-22 15:46   ` Guenter Roeck
  2021-02-28 11:50     ` Joe Perches
  0 siblings, 1 reply; 38+ messages in thread
From: Guenter Roeck @ 2021-02-22 15:46 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening; +Cc: linux-hwmon, linux-kernel

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);
>  }
>  
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 20/20] s390/watchdog: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 20/20] s390/watchdog: " Romain Perier
@ 2021-02-22 15:55   ` Guenter Roeck
  0 siblings, 0 replies; 38+ messages in thread
From: Guenter Roeck @ 2021-02-22 15:55 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Wim Van Sebroeck
  Cc: linux-watchdog, linux-kernel

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);
>  
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 14/20] target: " Romain Perier
@ 2021-02-22 16:00   ` Bodo Stroesser
  2021-02-22 18:09   ` kernel test robot
  1 sibling, 0 replies; 38+ messages in thread
From: Bodo Stroesser @ 2021-02-22 16:00 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Martin K. Petersen
  Cc: linux-scsi, target-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 13/20] scsi: zfcp: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
@ 2021-02-22 16:04   ` Benjamin Block
  0 siblings, 0 replies; 38+ messages in thread
From: Benjamin Block @ 2021-02-22 16:04 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, Steffen Maier, linux-s390, linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 19/20] usbip: usbip_host: " Romain Perier
@ 2021-02-22 16:21   ` Shuah Khan
  2021-02-28  9:03   ` Sergei Shtylyov
  1 sibling, 0 replies; 38+ messages in thread
From: Shuah Khan @ 2021-02-22 16:21 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Greg Kroah-Hartman,
	Valentina Manea, Shuah Khan
  Cc: linux-usb, linux-kernel, 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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy
  2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
                   ` (19 preceding siblings ...)
  2021-02-22 15:12 ` [PATCH 20/20] s390/watchdog: " Romain Perier
@ 2021-02-22 16:36 ` Shuah Khan
  20 siblings, 0 replies; 38+ messages in thread
From: Shuah Khan @ 2021-02-22 16:36 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Tejun Heo, Zefan Li,
	Johannes Weiner, Herbert Xu, David S. Miller, Jiri Pirko,
	Sumit Semwal, Christian König, Greg Kroah-Hartman,
	Mimi Zohar, Dmitry Kasatkin, J. Bruce Fields, Chuck Lever,
	Geert Uytterhoeven, Jessica Yu, Guenter Roeck, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Steffen Maier,
	Benjamin Block, Martin K. Petersen, Jaroslav Kysela,
	Takashi Iwai, Steven Rostedt, Ingo Molnar, Jiri Slaby,
	Felipe Balbi, Valentina Manea, Shuah Khan, Wim Van Sebroeck
  Cc: cgroups, linux-crypto, netdev, linux-media, dri-devel,
	linaro-mm-sig, Rafael J. Wysocki, linux-integrity, linux-nfs,
	linux-m68k, linux-hwmon, linux-s390, linux-scsi, target-devel,
	alsa-devel, linux-usb, linux-watchdog, linux-kernel, 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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 16/20] tracing/probe: " Romain Perier
@ 2021-02-22 17:49   ` Steven Rostedt
  0 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2021-02-22 17:49 UTC (permalink / raw)
  To: Romain Perier; +Cc: Kees Cook, kernel-hardening, Ingo Molnar, linux-kernel

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)


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 14/20] target: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 14/20] target: " Romain Perier
  2021-02-22 16:00   ` Bodo Stroesser
@ 2021-02-22 18:09   ` kernel test robot
  1 sibling, 0 replies; 38+ messages in thread
From: kernel test robot @ 2021-02-22 18:09 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Martin K. Petersen
  Cc: kbuild-all, Romain Perier, linux-scsi, target-devel, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 03/20] devlink: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 03/20] devlink: " Romain Perier
@ 2021-02-23  0:56   ` Jakub Kicinski
  0 siblings, 0 replies; 38+ messages in thread
From: Jakub Kicinski @ 2021-02-23  0:56 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, Jiri Pirko, netdev, linux-kernel

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) ?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values Romain Perier
@ 2021-02-23 16:13   ` Michal Koutný
  0 siblings, 0 replies; 38+ messages in thread
From: Michal Koutný @ 2021-02-23 16:13 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, Tejun Heo, Zefan Li,
	Johannes Weiner, cgroups, linux-kernel

[-- 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 --]

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 17/20] vt: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 17/20] vt: " Romain Perier
@ 2021-02-26  9:49   ` Jiri Slaby
  0 siblings, 0 replies; 38+ messages in thread
From: Jiri Slaby @ 2021-02-26  9:49 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Greg Kroah-Hartman
  Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 19/20] usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 19/20] usbip: usbip_host: " Romain Perier
  2021-02-22 16:21   ` Shuah Khan
@ 2021-02-28  9:03   ` Sergei Shtylyov
  1 sibling, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2021-02-28  9:03 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Greg Kroah-Hartman,
	Valentina Manea, Shuah Khan, Shuah Khan
  Cc: linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:46   ` Guenter Roeck
@ 2021-02-28 11:50     ` Joe Perches
  0 siblings, 0 replies; 38+ messages in thread
From: Joe Perches @ 2021-02-28 11:50 UTC (permalink / raw)
  To: Guenter Roeck, Romain Perier, Kees Cook, kernel-hardening
  Cc: linux-hwmon, linux-kernel

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/



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 07/20] SUNRPC: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 07/20] SUNRPC: " Romain Perier
@ 2021-03-01 18:25   ` Chuck Lever
  0 siblings, 0 replies; 38+ messages in thread
From: Chuck Lever @ 2021-03-01 18:25 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, Bruce Fields,
	Linux NFS Mailing List, linux-kernel



> 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




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 06/20] ima: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 06/20] ima: " Romain Perier
@ 2021-03-02 13:29   ` Mimi Zohar
  0 siblings, 0 replies; 38+ messages in thread
From: Mimi Zohar @ 2021-03-02 13:29 UTC (permalink / raw)
  To: Romain Perier, Kees Cook, kernel-hardening, Dmitry Kasatkin
  Cc: linux-integrity, linux-kernel

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]);
> 




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 02/20] crypto: Manual replacement of the deprecated strlcpy() with return values
  2021-02-22 15:12 ` [PATCH 02/20] crypto: " Romain Perier
@ 2021-03-04  4:37   ` Herbert Xu
  0 siblings, 0 replies; 38+ messages in thread
From: Herbert Xu @ 2021-03-04  4:37 UTC (permalink / raw)
  To: Romain Perier
  Cc: Kees Cook, kernel-hardening, David S. Miller, linux-crypto, linux-kernel

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2021-03-04  4:39 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-22 15:12 [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Romain Perier
2021-02-22 15:12 ` [PATCH 01/20] cgroup: Manual replacement of the deprecated strlcpy() with return values Romain Perier
2021-02-23 16:13   ` Michal Koutný
2021-02-22 15:12 ` [PATCH 02/20] crypto: " Romain Perier
2021-03-04  4:37   ` Herbert Xu
2021-02-22 15:12 ` [PATCH 03/20] devlink: " Romain Perier
2021-02-23  0:56   ` Jakub Kicinski
2021-02-22 15:12 ` [PATCH 04/20] dma-buf: " Romain Perier
2021-02-22 15:12 ` [PATCH 05/20] kobject: " Romain Perier
2021-02-22 15:12 ` [PATCH 06/20] ima: " Romain Perier
2021-03-02 13:29   ` Mimi Zohar
2021-02-22 15:12 ` [PATCH 07/20] SUNRPC: " Romain Perier
2021-03-01 18:25   ` Chuck Lever
2021-02-22 15:12 ` [PATCH 08/20] kernfs: " Romain Perier
2021-02-22 15:12 ` [PATCH 09/20] m68k/atari: " Romain Perier
2021-02-22 15:12 ` [PATCH 10/20] module: " Romain Perier
2021-02-22 15:12 ` [PATCH 11/20] hwmon: " Romain Perier
2021-02-22 15:46   ` Guenter Roeck
2021-02-28 11:50     ` Joe Perches
2021-02-22 15:12 ` [PATCH 12/20] s390/hmcdrv: " Romain Perier
2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
2021-02-22 16:04   ` Benjamin Block
2021-02-22 15:12 ` [PATCH 14/20] target: " Romain Perier
2021-02-22 16:00   ` Bodo Stroesser
2021-02-22 18:09   ` kernel test robot
2021-02-22 15:12 ` [PATCH 15/20] ALSA: usb-audio: " Romain Perier
2021-02-22 15:39   ` Takashi Iwai
2021-02-22 15:12 ` [PATCH 16/20] tracing/probe: " Romain Perier
2021-02-22 17:49   ` Steven Rostedt
2021-02-22 15:12 ` [PATCH 17/20] vt: " Romain Perier
2021-02-26  9:49   ` Jiri Slaby
2021-02-22 15:12 ` [PATCH 18/20] usb: gadget: f_midi: " Romain Perier
2021-02-22 15:12 ` [PATCH 19/20] usbip: usbip_host: " Romain Perier
2021-02-22 16:21   ` Shuah Khan
2021-02-28  9:03   ` Sergei Shtylyov
2021-02-22 15:12 ` [PATCH 20/20] s390/watchdog: " Romain Perier
2021-02-22 15:55   ` Guenter Roeck
2021-02-22 16:36 ` [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Shuah Khan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).