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
