[PATCH v2] ceph: Use strscpy() instead of strcpy()

[PATCH v2] ceph: Use strscpy() instead of strcpy()

[Abdul Rahim]

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

Re: [PATCH v2] ceph: Use strscpy() instead of strcpy()

[Ilya Dryomov]

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