* [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 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values Romain Perier
` (2 more replies)
0 siblings, 3 replies; 5+ 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] 5+ 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
@ 2021-02-22 15:12 ` Romain Perier
2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
2021-02-22 16:36 ` [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Shuah Khan
2 siblings, 0 replies; 5+ 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] 5+ 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
2021-02-22 15:12 ` [PATCH 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values Romain Perier
@ 2021-02-22 15:12 ` Romain Perier
2021-02-22 16:04 ` Benjamin Block
2021-02-22 16:36 ` [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy Shuah Khan
2 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ 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
2021-02-22 15:12 ` [PATCH 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values Romain Perier
2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
@ 2021-02-22 16:36 ` Shuah Khan
2 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2021-02-22 16:37 UTC | newest]
Thread overview: 5+ 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 12/20] s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values Romain Perier
2021-02-22 15:12 ` [PATCH 13/20] scsi: zfcp: " Romain Perier
2021-02-22 16:04 ` Benjamin Block
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).