| Message ID | 20250405143646.10722-1-goralbaris@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] scsi: target: transform strncpy into strscpy | expand |
On Sat, 5 Apr 2025 17:36:47 +0300 Baris Can Goral <goralbaris@gmail.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@gmail.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); > } >
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@gmail.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@gmail.com>, 5 Nis 2025 Cmt, 18:25 tarihinde şunu yazdı: >> >> On Sat, 5 Apr 2025 17:36:47 +0300 >> Baris Can Goral <goralbaris@gmail.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@gmail.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); >> > } >> > >>
On Sat, 5 Apr 2025 19:35:01 +0300 baris goral <goralbaris@gmail.com> wrote: > Hi, > Trying to understand, it has if check a few lines above: > > if (count > (DB_ROOT_LEN - 1)) > > Does not it met our expectations? Don't top post on mailing lists. The first issue is that the return value of snprintf() is the number of characters that would be written into the buffer were it long enough. The kernel's scnprintf() will return the number of characters written. But why is it using snprintf() just to copy a string? Why is truncation at all safe here? Why is a '\n' being removed without the length being changed. The length argument to strscpy() should be the length of the destination (to stop overruns), not the number of characters. In this case it is the number of characters - so will delete another character (unless a '\n' was removed). The return value is just garbage. You may have opened a bag of worms, but you've also made it worse. David > > David Laight <david.laight.linux@gmail.com>, 5 Nis 2025 Cmt, 18:25 > tarihinde şunu yazdı: > > > On Sat, 5 Apr 2025 17:36:47 +0300 > > Baris Can Goral <goralbaris@gmail.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@gmail.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); > > > } > > > > > > >
On Sat Apr 5, 2025 at 4:36 PM CEST, Baris Can Goral 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@gmail.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); > > 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); > } > This patch doesn't apply anymore. strncpy() has already been replaced with strscpy() in version 6.14-rc2. Maurizio
Hi Maurizio, Unfourtunately, in version 6.14-rc2 (and also v6.14) strncpy is still there. Best Regards, Baris
On Mon Apr 7, 2025 at 7:48 PM CEST, Baris Can Goral wrote: > Hi Maurizio, > > Unfourtunately, in version 6.14-rc2 (and also v6.14) strncpy is still there. Sorry, my mistake. commit dfb7df1ddb29c89662e84b2c82c1ff7943358ae0 that replaces strncpy() with strscpy() has been merged in kernel version 6.15-rc1 Maurizio
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); 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); }
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@gmail.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(-)