Re: [PATCH] null_blk: refactor deprecated strncpy

[Kees Cook <keescook@chromium.org>]

On Mon, Sep 11, 2023 at 09:46:47PM +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 know `nullb->disk_name` is NUL-terminated and we expect that
> `disk->disk_name` also be NUL-terminated:
> |     snprintf(nullb->disk_name, sizeof(nullb->disk_name),
> |              "%s", config_item_name(&dev->group.cg_item));

More accurately, it's has uses that require a %NUL-terminated string:

	pr_info("disk %s created\n", nullb->disk_name);

(i.e. showing the consumer's use is better evidence than the producer's
use.)

But I do think the above is good evidence that truncation is tolerated.

> 
> 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);
> | }

I does look like it expects 0-fill. Looking at it with more context,
this appears to be a trace buffer:

TRACE_EVENT(nullb_zone_op,
            TP_PROTO(struct nullb_cmd *cmd, unsigned int zone_no,
                     unsigned int zone_cond),
            TP_ARGS(cmd, zone_no, zone_cond),
            TP_STRUCT__entry(
                __array(char, disk, DISK_NAME_LEN)
                __field(enum req_op, op)
                __field(unsigned int, zone_no)
                __field(unsigned int, zone_cond)
            ),
            TP_fast_assign(
                __entry->op = req_op(cmd->rq);
                __entry->zone_no = zone_no;
                __entry->zone_cond = zone_cond;
                __assign_disk_name(__entry->disk, cmd->rq->q->disk);
            ),

This should probably have been a dynamic string, but it's not. So let's
make sure this stays padded. Can you send this again but use
strscpy_pad() instead?

-- 
Kees Cook