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
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 <[email protected]>
---
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);
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 <[email protected]>
---
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]);
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 <[email protected]>
---
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);
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 <[email protected]>
---
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",
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 <[email protected]>
---
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));
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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;
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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);
}
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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));
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 <[email protected]>
---
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)) {
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 <[email protected]>
---
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
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 <[email protected]>
---
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);
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 <[email protected]>
---
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)
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 <[email protected]>
---
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;
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 <[email protected]>
---
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';
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 <[email protected]>
---
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");
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 <[email protected]>
The strlcpy() usage in sound/* have been already converted on the
latest Linus tree. So please drop this one.
thanks,
Takashi
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 <[email protected]>
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);
> }
>
>
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
thanks,
-- Shuah
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
On Mon, 22 Feb 2021 16:12:27 +0100
Romain Perier <[email protected]> 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 <[email protected]>
> ---
> 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)
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 <[email protected]>
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/[email protected]
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) ?
Hello.
On Mon, Feb 22, 2021 at 04:12:12PM +0100, Romain Perier <[email protected]> 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?= <[email protected]>
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 <[email protected]>
Signed-off-by: Michal Koutn? <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
[...]
MBR, Sergei
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 <[email protected]>
>
> 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/[email protected]/T/
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 <[email protected]>
> ---
> 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 <[email protected]>
thanks,
Mimi
> + continue;
> + }
>
> INIT_LIST_HEAD(&arch_policy_entry[i].list);
> result = ima_parse_rule(rule, &arch_policy_entry[i]);
>