linux-kernel - Re: [RFC PATCH v2 1/1] kernfs: replace deprecated strlcpy() with strscpy()

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

Message-ID: <ZV5zQ0gRxzGwweYN@slm.duckdns.org>
Date:   Wed, 22 Nov 2023 11:31:47 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Konstantin Khlebnikov <koct9i@...il.com>,
        Aditya Kali <adityakali@...gle.com>
Subject: Re: [RFC PATCH v2 1/1] kernfs: replace deprecated strlcpy() with
 strscpy()

Hello,

On Wed, Nov 22, 2023 at 10:20:10PM +0100, Mirsad Goran Todorovac wrote:
> From: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
> 
> According to strlcpy() being officially deprecated and the encouragement
> to remove the remaining occurrences, this came as the intriguing example.
> 
> In the kernfs_name_locked() the behaviour of truncating the kn->name is
> preserved, for it only used in the module for printing in the log and
> declared static. It is only called from pr_cont_kernfs_name() via kernfs_name()
> and returned result is ignored.
> 
> It is avoided to go past the allocated page and cause the internal equivalent
> of SEGFAULT in the unlikely case kn->name is not null-terminated, which I
> believe was the idea behind replacing strlcpy() with strscpy().

I don't follow this line of thinking. Yeah, if the input arguments are
wrong, it can malfunction. strscpy() is gonna page fault too if the input
string crosses into an unmapped page before the destination buffer is
filled, right? But it'd be a stretch to claim that that's a problem with the
function. Maybe I'm missing something but I'm having a hard time seeing the
value in these conversions.

> kernfs_path_from_node_locked() has "(null)" which certainly cannot overrun,
> and a carefully calculated len and truncated path elsewhere.

Some of the functions you're modifying were returning the full length of the
input string before and it isn't clear from the patch or description whether
the conversion is safe. It'd help if you can elaborate more on why the
conversions are safe and how you verified them.

Thanks.

-- 
tejun