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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOi1vP9dCx5mpjZ3RpKZ5z1zYYW9SbH8DWWtRD9J0xcEmerFpw@mail.gmail.com>
Date: Mon, 18 Nov 2024 17:25:55 +0100
From: Ilya Dryomov <idryomov@...il.com>
To: Abdul Rahim <abdul.rahim@...ahoo.com>
Cc: xiubli@...hat.com, ceph-devel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ceph: Use strscpy() instead of strcpy()

On Fri, Nov 15, 2024 at 12:24 PM Abdul Rahim <abdul.rahim@...ahoo.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@...ahoo.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@...adoo.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