linux-hardening - Re: [PATCH] null_blk: refactor deprecated strncpy

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

Message-ID: <202309142022.50EF929@keescook>
Date:   Thu, 14 Sep 2023 20:27:59 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
Cc:     Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH] null_blk: refactor deprecated strncpy

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