linux-kernel - Re: [PATCH v4] scsi: target: transform strncpy into strscpy

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>] [day] [month] [year] [list]

Message-ID: <CAJOJxiyOZgRrf=e3tJuKiAdZP4U4MS4Y0EQFd5HPYAHgUhXVww@mail.gmail.com>
Date: Sat, 5 Apr 2025 20:28:42 +0300
From: baris goral <goralbaris@...il.com>
To: David Laight <david.laight.linux@...il.com>
Cc: martin.petersen@...cle.com, linux-scsi@...r.kernel.org, 
	target-devel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH v4] scsi: target: transform strncpy into strscpy

Hi,
Trying to understand, it has if check a few lines above:

if (count > (DB_ROOT_LEN - 1))

Does not it met our expectations?


Best Reagrds,
Baris


baris goral <goralbaris@...il.com>, 5 Nis 2025 Cmt, 19:35 tarihinde şunu yazdı:
>
> Hi,
> Trying to understand, it has if check a few lines above:
>
> if (count > (DB_ROOT_LEN - 1))
>
> Does not it met our expectations?
>
>
> David Laight <david.laight.linux@...il.com>, 5 Nis 2025 Cmt, 18:25 tarihinde şunu yazdı:
>>
>> On Sat,  5 Apr 2025 17:36:47 +0300
>> Baris Can Goral <goralbaris@...il.com> wrote:
>>
>> > The strncpy() function is actively dangerous to use since it may not
>> > NULL-terminate the destination string,resulting in potential memory
>> > content exposures, unbounded reads, or crashes.
>> >
>> > Link:https://github.com/KSPP/linux/issues/90
>> > Signed-off-by: Baris Can Goral <goralbaris@...il.com>
>> > ---
>> > Changes from v4:
>> >       -Description added
>> >       -User name corrected
>> >       -formatting issues.
>> >       -commit name changed
>> >  drivers/target/target_core_configfs.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> > index c40217f44b1b..5c0b74e76be2 100644
>> > --- a/drivers/target/target_core_configfs.c
>> > +++ b/drivers/target/target_core_configfs.c
>> > @@ -143,7 +143,7 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
>> >       }
>> >       filp_close(fp, NULL);
>> >
>> > -     strncpy(db_root, db_root_stage, read_bytes);
>> > +     strscpy(db_root, db_root_stage, read_bytes);
>> >       pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>>
>> That code is broken, it reads:
>>         read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
>>         if (!read_bytes)
>>                 goto unlock;
>>
>>         if (db_root_stage[read_bytes - 1] == '\n')
>>                 db_root_stage[read_bytes - 1] = '\0';
>>
>>         /* validate new db root before accepting it */
>>         fp = filp_open(db_root_stage, O_RDONLY, 0);
>>         if (IS_ERR(fp)) {
>>                 pr_err("db_root: cannot open: %s\n", db_root_stage);
>>                 goto unlock;
>>         }
>>         if (!S_ISDIR(file_inode(fp)->i_mode)) {
>>                 filp_close(fp, NULL);
>>                 pr_err("db_root: not a directory: %s\n", db_root_stage);
>>                 goto unlock;
>>         }
>>         filp_close(fp, NULL);
>>
>>         strncpy(db_root, db_root_stage, read_bytes);
>>         pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>>
>>         r = read_bytes;
>>
>> unlock:
>>         mutex_unlock(&target_devices_lock);
>>         return r;
>>
>> 'Really nasty (tm)' things happen if 'page' is too long.
>>
>>         David
>>
>> >
>> >       r = read_bytes;
>> > @@ -3664,7 +3664,7 @@ static void target_init_dbroot(void)
>> >       }
>> >       filp_close(fp, NULL);
>> >
>> > -     strncpy(db_root, db_root_stage, DB_ROOT_LEN);
>> > +     strscpy(db_root, db_root_stage, DB_ROOT_LEN);
>> >       pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>> >  }
>> >
>>