Skip to content

Commit 6291716

Browse files
JustinStittbrauner
authored andcommitted
orangefs: cleanup uses of strncpy
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. There is some care taken to ensure these destination buffers are NUL-terminated by bounding the strncpy()'s by ORANGEFS_NAME_MAX - 1 or ORANGEFS_MAX_SERVER_ADDR_LEN - 1. Instead, we can use the new 2-argument version of strscpy() to guarantee NUL-termination on the destination buffers while simplifying the code. Based on usage with printf-likes, we can see these buffers are expected to be NUL-terminated: | gossip_debug(GOSSIP_NAME_DEBUG, | "%s: doing lookup on %s under %pU,%d\n", | __func__, | new_op->upcall.req.lookup.d_name, | &new_op->upcall.req.lookup.parent_refn.khandle, | new_op->upcall.req.lookup.parent_refn.fs_id); ... | gossip_debug(GOSSIP_SUPER_DEBUG, | "Attempting ORANGEFS Remount via host %s\n", | new_op->upcall.req.fs_mount.orangefs_config_server); NUL-padding isn't required for any of these destination buffers as they've all been zero-allocated with op_alloc() or kzalloc(). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: KSPP#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Link: https://lore.kernel.org/r/20240322-strncpy-fs-orangefs-dcache-c-v1-1-15d12debbf38@google.com Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Christian Brauner <brauner@kernel.org>
1 parent c473bcd commit 6291716

3 files changed

Lines changed: 15 additions & 32 deletions

File tree

fs/orangefs/dcache.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,7 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
3333

3434
new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
3535
new_op->upcall.req.lookup.parent_refn = parent->refn;
36-
strncpy(new_op->upcall.req.lookup.d_name,
37-
dentry->d_name.name,
38-
ORANGEFS_NAME_MAX - 1);
36+
strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
3937

4038
gossip_debug(GOSSIP_DCACHE_DEBUG,
4139
"%s:%s:%d interrupt flag [%d]\n",

fs/orangefs/namei.c

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,7 @@ static int orangefs_create(struct mnt_idmap *idmap,
4141
fill_default_sys_attrs(new_op->upcall.req.create.attributes,
4242
ORANGEFS_TYPE_METAFILE, mode);
4343

44-
strncpy(new_op->upcall.req.create.d_name,
45-
dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
44+
strscpy(new_op->upcall.req.create.d_name, dentry->d_name.name);
4645

4746
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
4847

@@ -137,8 +136,7 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
137136
&parent->refn.khandle);
138137
new_op->upcall.req.lookup.parent_refn = parent->refn;
139138

140-
strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
141-
ORANGEFS_NAME_MAX - 1);
139+
strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name);
142140

143141
gossip_debug(GOSSIP_NAME_DEBUG,
144142
"%s: doing lookup on %s under %pU,%d\n",
@@ -192,8 +190,7 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
192190
return -ENOMEM;
193191

194192
new_op->upcall.req.remove.parent_refn = parent->refn;
195-
strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
196-
ORANGEFS_NAME_MAX - 1);
193+
strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name);
197194

198195
ret = service_operation(new_op, "orangefs_unlink",
199196
get_interruptible_flag(inode));
@@ -247,10 +244,8 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
247244
ORANGEFS_TYPE_SYMLINK,
248245
mode);
249246

250-
strncpy(new_op->upcall.req.sym.entry_name,
251-
dentry->d_name.name,
252-
ORANGEFS_NAME_MAX - 1);
253-
strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
247+
strscpy(new_op->upcall.req.sym.entry_name, dentry->d_name.name);
248+
strscpy(new_op->upcall.req.sym.target, symname);
254249

255250
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
256251

@@ -324,8 +319,7 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
324319
fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
325320
ORANGEFS_TYPE_DIRECTORY, mode);
326321

327-
strncpy(new_op->upcall.req.mkdir.d_name,
328-
dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
322+
strscpy(new_op->upcall.req.mkdir.d_name, dentry->d_name.name);
329323

330324
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
331325

@@ -405,12 +399,8 @@ static int orangefs_rename(struct mnt_idmap *idmap,
405399
new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
406400
new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
407401

408-
strncpy(new_op->upcall.req.rename.d_old_name,
409-
old_dentry->d_name.name,
410-
ORANGEFS_NAME_MAX - 1);
411-
strncpy(new_op->upcall.req.rename.d_new_name,
412-
new_dentry->d_name.name,
413-
ORANGEFS_NAME_MAX - 1);
402+
strscpy(new_op->upcall.req.rename.d_old_name, old_dentry->d_name.name);
403+
strscpy(new_op->upcall.req.rename.d_new_name, new_dentry->d_name.name);
414404

415405
ret = service_operation(new_op,
416406
"orangefs_rename",

fs/orangefs/super.c

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -253,9 +253,8 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
253253
new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
254254
if (!new_op)
255255
return -ENOMEM;
256-
strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
257-
orangefs_sb->devname,
258-
ORANGEFS_MAX_SERVER_ADDR_LEN);
256+
strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
257+
orangefs_sb->devname);
259258

260259
gossip_debug(GOSSIP_SUPER_DEBUG,
261260
"Attempting ORANGEFS Remount via host %s\n",
@@ -400,8 +399,7 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
400399
return -ENOMEM;
401400
op->upcall.req.fs_umount.id = id;
402401
op->upcall.req.fs_umount.fs_id = fs_id;
403-
strncpy(op->upcall.req.fs_umount.orangefs_config_server,
404-
devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
402+
strscpy(op->upcall.req.fs_umount.orangefs_config_server, devname);
405403
r = service_operation(op, "orangefs_fs_umount", 0);
406404
/* Not much to do about an error here. */
407405
if (r)
@@ -494,9 +492,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
494492
if (!new_op)
495493
return ERR_PTR(-ENOMEM);
496494

497-
strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
498-
devname,
499-
ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
495+
strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, devname);
500496

501497
gossip_debug(GOSSIP_SUPER_DEBUG,
502498
"Attempting ORANGEFS Mount via host %s\n",
@@ -543,9 +539,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
543539
* on successful mount, store the devname and data
544540
* used
545541
*/
546-
strncpy(ORANGEFS_SB(sb)->devname,
547-
devname,
548-
ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
542+
strscpy(ORANGEFS_SB(sb)->devname, devname);
543+
549544

550545
/* mount_pending must be cleared */
551546
ORANGEFS_SB(sb)->mount_pending = 0;

0 commit comments

Comments
 (0)