| Message ID | 20241115112419.11137-1-abdul.rahim@myyahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ceph: Use strscpy() instead of strcpy() | expand |
On Fri, Nov 15, 2024 at 12:24 PM Abdul Rahim <abdul.rahim@myyahoo.com> wrote: > > strcpy() performs no bounds checking on the destination buffer. This > could result in linear overflows beyond the end of the buffer, leading > to all kinds of misbehaviors. [1] > > this fixes checkpatch warning: > WARNING: Prefer strscpy over strcpy > > [1] : https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy > Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com> > --- > Changes since v1: > - Added third parameter in strscpy() > - Added comment to explain where the limit `NAME_MAX+1` is comming from > as suggested by Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > Link to v1: https://lore.kernel.org/lkml/20241111221037.92853-1-abdul.rahim@myyahoo.com/ > > The function __get_snap_name() is assigned to .get_name() from > struct export_operations, when `ceph_snap(inode) != CEPH_NOSNAP`. > `struct export_operations` is comming from `include/linux/exportfs.h`, > and according to [1], the operation get_name assumes that the variable > `name` is pointing to a buffer of size NAME_MAX+1 > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203 > > fs/ceph/export.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index 44451749c544..96421f2b6cec 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -452,7 +452,11 @@ static int __get_snap_name(struct dentry *parent, char *name, > goto out; > if (ceph_snap(inode) == CEPH_SNAPDIR) { > if (ceph_snap(dir) == CEPH_NOSNAP) { > - strcpy(name, fsc->mount_options->snapdir_name); > + /* .get_name() from struct export_operations assumes > + * that its 'name' parameter is pointing to a > + * NAME_MAX+1 sized buffer */ > + strscpy(name, fsc->mount_options->snapdir_name, > + NAME_MAX+1); > err = 0; > } > goto out; > -- > 2.43.0 > Applied with minor formatting tweaks. Thanks, Ilya
diff --git a/fs/ceph/export.c b/fs/ceph/export.c index 44451749c544..96421f2b6cec 100644 --- a/fs/ceph/export.c +++ b/fs/ceph/export.c @@ -452,7 +452,11 @@ static int __get_snap_name(struct dentry *parent, char *name, goto out; if (ceph_snap(inode) == CEPH_SNAPDIR) { if (ceph_snap(dir) == CEPH_NOSNAP) { - strcpy(name, fsc->mount_options->snapdir_name); + /* .get_name() from struct export_operations assumes + * that its 'name' parameter is pointing to a + * NAME_MAX+1 sized buffer */ + strscpy(name, fsc->mount_options->snapdir_name, + NAME_MAX+1); err = 0; } goto out;
strcpy() performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. [1] this fixes checkpatch warning: WARNING: Prefer strscpy over strcpy [1] : https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com> --- Changes since v1: - Added third parameter in strscpy() - Added comment to explain where the limit `NAME_MAX+1` is comming from as suggested by Christophe JAILLET <christophe.jaillet@wanadoo.fr> Link to v1: https://lore.kernel.org/lkml/20241111221037.92853-1-abdul.rahim@myyahoo.com/ The function __get_snap_name() is assigned to .get_name() from struct export_operations, when `ceph_snap(inode) != CEPH_NOSNAP`. `struct export_operations` is comming from `include/linux/exportfs.h`, and according to [1], the operation get_name assumes that the variable `name` is pointing to a buffer of size NAME_MAX+1 [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203 fs/ceph/export.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)