| Message ID | 20240206142221.2208763-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 1e06589843632afc78d2f8e9735dac3146b97988 |
| Headers | show |
| Series | string: Allow 2-argument strscpy() | expand |
On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@chromium.org> wrote: > > The ARCH=um build has its own idea about strscpy()'s definition. Adjust > the callers to remove the redundant sizeof() arguments ahead of treewide > changes, since it needs a manual adjustment for the newly named > sized_strscpy() export. ... > - strscpy(dir, home, sizeof(dir)); > + strscpy(dir, home); > uml_dir++; > } > strlcat(dir, uml_dir, sizeof(dir)); An (unrelated) side note: are we going to get rid of strlcat() as well (after strlcpy() is gone)? ... > if (*umid == '\0') { > - strscpy(tmp, uml_dir, sizeof(tmp)); > + strscpy(tmp, uml_dir); > strlcat(tmp, "XXXXXX", sizeof(tmp)); This code is interesting... (Esp. taking into account making a temporary folder out of this...)
On Tue, Feb 06, 2024 at 05:02:16PM +0200, Andy Shevchenko wrote: > On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@chromium.org> wrote: > > > > The ARCH=um build has its own idea about strscpy()'s definition. Adjust > > the callers to remove the redundant sizeof() arguments ahead of treewide > > changes, since it needs a manual adjustment for the newly named > > sized_strscpy() export. > > ... > > > - strscpy(dir, home, sizeof(dir)); > > + strscpy(dir, home); > > uml_dir++; > > } > > strlcat(dir, uml_dir, sizeof(dir)); > > An (unrelated) side note: are we going to get rid of strlcat() as well > (after strlcpy() is gone)? I think it would be worthwhile to remove it, yes. Switching to seq_buf in many cases seemed to be the clear solution, which is what triggered my trying to improve the allocation ergonomics for seq_buf recently: https://lore.kernel.org/linux-hardening/20231027155634.make.260-kees@kernel.org/ Its not in super common usage, so I think we can start chipping away at it: $ git grep '\bstrlcat(' | wc -l 480 It's more risky cases (using the return value) is relatively rare, though, so I hadn't been prioritizing its removal: $ git grep ' = strlcat(\b' | wc -l 13 (And almost all of it is in security/selinux/ima.c) As a comparison, strncpy has even fewer currently, with Justin making a dent on it recently: $ git grep '\bstrncpy(' | wc -l 311 > > ... > > > if (*umid == '\0') { > > - strscpy(tmp, uml_dir, sizeof(tmp)); > > + strscpy(tmp, uml_dir); > > strlcat(tmp, "XXXXXX", sizeof(tmp)); > > This code is interesting... (Esp. taking into account making a > temporary folder out of this...) I have tried to avoid reading UML code too closely. ;)
diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c index cabcc501b448..77c4afb8ab90 100644 --- a/arch/um/drivers/net_kern.c +++ b/arch/um/drivers/net_kern.c @@ -265,7 +265,7 @@ static void uml_net_poll_controller(struct net_device *dev) static void uml_net_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info) { - strscpy(info->driver, DRIVER_NAME, sizeof(info->driver)); + strscpy(info->driver, DRIVER_NAME); } static const struct ethtool_ops uml_net_ethtool_ops = { diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c index 131b7cb29576..dc2feae789cb 100644 --- a/arch/um/drivers/vector_kern.c +++ b/arch/um/drivers/vector_kern.c @@ -1373,7 +1373,7 @@ static void vector_net_poll_controller(struct net_device *dev) static void vector_net_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info) { - strscpy(info->driver, DRIVER_NAME, sizeof(info->driver)); + strscpy(info->driver, DRIVER_NAME); } static int vector_net_load_bpf_flash(struct net_device *dev, diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c index c719e1ec4645..b16a5e5619d3 100644 --- a/arch/um/drivers/vector_user.c +++ b/arch/um/drivers/vector_user.c @@ -141,7 +141,7 @@ static int create_tap_fd(char *iface) } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR; - strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); + strscpy(ifr.ifr_name, iface); err = ioctl(fd, TUNSETIFF, (void *) &ifr); if (err != 0) { @@ -171,7 +171,7 @@ static int create_raw_fd(char *iface, int flags, int proto) goto raw_fd_cleanup; } memset(&ifr, 0, sizeof(ifr)); - strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); + strscpy(ifr.ifr_name, iface); if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) { err = -errno; goto raw_fd_cleanup; diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h index 9568cc04cbb7..326e52450e41 100644 --- a/arch/um/include/shared/user.h +++ b/arch/um/include/shared/user.h @@ -52,7 +52,7 @@ static inline int printk(const char *fmt, ...) extern int in_aton(char *str); extern size_t strlcat(char *, const char *, size_t); extern size_t sized_strscpy(char *, const char *, size_t); -#define strscpy(dst, src, size) sized_strscpy(dst, src, size) +#define strscpy(dst, src) sized_strscpy(dst, src, sizeof(dst)) /* Copied from linux/compiler-gcc.h since we can't include it directly */ #define barrier() __asm__ __volatile__("": : :"memory") diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c index 3363851a4ae8..bdf215c0eca7 100644 --- a/arch/um/os-Linux/drivers/ethertap_user.c +++ b/arch/um/os-Linux/drivers/ethertap_user.c @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me, sprintf(data_fd_buf, "%d", data_remote); sprintf(version_buf, "%d", UML_NET_VERSION); if (gate != NULL) { - strscpy(gate_buf, gate, sizeof(gate_buf)); + strscpy(gate_buf, gate); args = setup_args; } else args = nosetup_args; diff --git a/arch/um/os-Linux/drivers/tuntap_user.c b/arch/um/os-Linux/drivers/tuntap_user.c index 2284e9c1cbbb..91f0e27ca3a6 100644 --- a/arch/um/os-Linux/drivers/tuntap_user.c +++ b/arch/um/os-Linux/drivers/tuntap_user.c @@ -146,7 +146,7 @@ static int tuntap_open(void *data) } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; - strscpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name)); + strscpy(ifr.ifr_name, pri->dev_name); if (ioctl(pri->fd, TUNSETIFF, &ifr) < 0) { err = -errno; printk(UM_KERN_ERR "TUNSETIFF failed, errno = %d\n", diff --git a/arch/um/os-Linux/umid.c b/arch/um/os-Linux/umid.c index 288c422bfa96..e09d65b05d1c 100644 --- a/arch/um/os-Linux/umid.c +++ b/arch/um/os-Linux/umid.c @@ -40,7 +40,7 @@ static int __init make_uml_dir(void) __func__); goto err; } - strscpy(dir, home, sizeof(dir)); + strscpy(dir, home); uml_dir++; } strlcat(dir, uml_dir, sizeof(dir)); @@ -243,7 +243,7 @@ int __init set_umid(char *name) if (strlen(name) > UMID_LEN - 1) return -E2BIG; - strscpy(umid, name, sizeof(umid)); + strscpy(umid, name); return 0; } @@ -262,7 +262,7 @@ static int __init make_umid(void) make_uml_dir(); if (*umid == '\0') { - strscpy(tmp, uml_dir, sizeof(tmp)); + strscpy(tmp, uml_dir); strlcat(tmp, "XXXXXX", sizeof(tmp)); fd = mkstemp(tmp); if (fd < 0) {
The ARCH=um build has its own idea about strscpy()'s definition. Adjust the callers to remove the redundant sizeof() arguments ahead of treewide changes, since it needs a manual adjustment for the newly named sized_strscpy() export. Cc: Richard Weinberger <richard@nod.at> Cc: linux-um@lists.infradead.org Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/um/drivers/net_kern.c | 2 +- arch/um/drivers/vector_kern.c | 2 +- arch/um/drivers/vector_user.c | 4 ++-- arch/um/include/shared/user.h | 2 +- arch/um/os-Linux/drivers/ethertap_user.c | 2 +- arch/um/os-Linux/drivers/tuntap_user.c | 2 +- arch/um/os-Linux/umid.c | 6 +++--- 7 files changed, 10 insertions(+), 10 deletions(-)