`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should favor a more robust and less ambiguous interface.
We expect that both `nullb->disk_name` and `disk->disk_name` be
NUL-terminated:
| snprintf(nullb->disk_name, sizeof(nullb->disk_name),
| "%s", config_item_name(&dev->group.cg_item));
...
| pr_info("disk %s created\n", nullb->disk_name);
It seems like NUL-padding may be required due to __assign_disk_name()
utilizing a memcpy as opposed to a `str*cpy` api.
| static inline void __assign_disk_name(char *name, struct gendisk *disk)
| {
| if (disk)
| memcpy(name, disk->disk_name, DISK_NAME_LEN);
| else
| memset(name, 0, DISK_NAME_LEN);
| }
Then we go and print it with `__print_disk_name` which wraps `nullb_trace_disk_name()`.
| #define __print_disk_name(name) nullb_trace_disk_name(p, name)
This function obviously expects a NUL-terminated string.
| const char *nullb_trace_disk_name(struct trace_seq *p, char *name)
| {
| const char *ret = trace_seq_buffer_ptr(p);
|
| if (name && *name)
| trace_seq_printf(p, "disk=%s, ", name);
| trace_seq_putc(p, 0);
|
| return ret;
| }
From the above, we need both 1) a NUL-terminated string and 2) a
NUL-padded string. So, let's use strscpy_pad() as per Kees' suggestion
from v1.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Cc: Kees Cook <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
Changes in v3:
- reword subject line (thanks Jens)
- rebase onto 3669558bdf354cd352be955ef2764cde6a9bf5ec
- Link to v2: https://lore.kernel.org/r/20230918-strncpy-drivers-block-null_blk-main-c-v2-1-cd9b109edfdf@google.com
Changes in v2:
- use strscpy_pad (thanks Kees)
- Link to v1: https://lore.kernel.org/r/20230911-strncpy-drivers-block-null_blk-main-c-v1-1-3b3887e7fde4@google.com
---
Note: build-tested
---
drivers/block/null_blk/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 968090935eb2..6cd0614d8786 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1946,7 +1946,7 @@ static int null_gendisk_register(struct nullb *nullb)
else
disk->fops = &null_bio_ops;
disk->private_data = nullb;
- strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
+ strscpy_pad(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
if (nullb->dev->zoned) {
int ret = null_register_zoned_dev(nullb);
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230911-strncpy-drivers-block-null_blk-main-c-7349153e1c6a
Best regards,
--
Justin Stitt <[email protected]>
On Tue, Sep 19, 2023 at 05:30:35AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should favor a more robust and less ambiguous interface.
>
> We expect that both `nullb->disk_name` and `disk->disk_name` be
> NUL-terminated:
> | snprintf(nullb->disk_name, sizeof(nullb->disk_name),
> | "%s", config_item_name(&dev->group.cg_item));
> ...
> | pr_info("disk %s created\n", nullb->disk_name);
>
> It seems like NUL-padding may be required due to __assign_disk_name()
> utilizing a memcpy as opposed to a `str*cpy` api.
> | static inline void __assign_disk_name(char *name, struct gendisk *disk)
> | {
> | if (disk)
> | memcpy(name, disk->disk_name, DISK_NAME_LEN);
> | else
> | memset(name, 0, DISK_NAME_LEN);
> | }
>
> Then we go and print it with `__print_disk_name` which wraps `nullb_trace_disk_name()`.
> | #define __print_disk_name(name) nullb_trace_disk_name(p, name)
>
> This function obviously expects a NUL-terminated string.
> | const char *nullb_trace_disk_name(struct trace_seq *p, char *name)
> | {
> | const char *ret = trace_seq_buffer_ptr(p);
> |
> | if (name && *name)
> | trace_seq_printf(p, "disk=%s, ", name);
> | trace_seq_putc(p, 0);
> |
> | return ret;
> | }
>
> From the above, we need both 1) a NUL-terminated string and 2) a
> NUL-padded string. So, let's use strscpy_pad() as per Kees' suggestion
> from v1.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Cc: Kees Cook <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Thanks for the analysis in the commit log, and yeah, this looks right to
me.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Tue, Sep 19, 2023 at 05:30:35AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should favor a more robust and less ambiguous interface.
>
> We expect that both `nullb->disk_name` and `disk->disk_name` be
> NUL-terminated:
> | snprintf(nullb->disk_name, sizeof(nullb->disk_name),
> | "%s", config_item_name(&dev->group.cg_item));
> ...
> | pr_info("disk %s created\n", nullb->disk_name);
>
> It seems like NUL-padding may be required due to __assign_disk_name()
> utilizing a memcpy as opposed to a `str*cpy` api.
> | static inline void __assign_disk_name(char *name, struct gendisk *disk)
> | {
> | if (disk)
> | memcpy(name, disk->disk_name, DISK_NAME_LEN);
> | else
> | memset(name, 0, DISK_NAME_LEN);
> | }
>
> Then we go and print it with `__print_disk_name` which wraps `nullb_trace_disk_name()`.
> | #define __print_disk_name(name) nullb_trace_disk_name(p, name)
>
> This function obviously expects a NUL-terminated string.
> | const char *nullb_trace_disk_name(struct trace_seq *p, char *name)
> | {
> | const char *ret = trace_seq_buffer_ptr(p);
> |
> | if (name && *name)
> | trace_seq_printf(p, "disk=%s, ", name);
> | trace_seq_putc(p, 0);
> |
> | return ret;
> | }
>
> >From the above, we need both 1) a NUL-terminated string and 2) a
> NUL-padded string. So, let's use strscpy_pad() as per Kees' suggestion
> from v1.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Cc: Kees Cook <[email protected]>
> Cc: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Ping on this one too. Jens, can you pick this up?
Thanks!
-Kees
--
Kees Cook
On Tue, 19 Sep 2023 05:30:35 +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should favor a more robust and less ambiguous interface.
>
> We expect that both `nullb->disk_name` and `disk->disk_name` be
> NUL-terminated:
> | snprintf(nullb->disk_name, sizeof(nullb->disk_name),
> | "%s", config_item_name(&dev->group.cg_item));
> ...
> | pr_info("disk %s created\n", nullb->disk_name);
>
> [...]
Applied, thanks!
[1/1] null_blk: replace strncpy with strscpy
commit: e1f2760ba3478fa3f8c48db8e91fbe3df8bea2f8
Best regards,
--
Jens Axboe