[v3,4/4] um: Convert strscpy() usage to 2-argument style
Commit Message
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(-)
Comments
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. ;)
@@ -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 = {
@@ -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,
@@ -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;
@@ -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")
@@ -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;
@@ -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",
@@ -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) {