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]

Date:   Sat, 25 Nov 2023 09:35:34 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Mirsad 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 Thu, Nov 23, 2023 at 12:37:03AM +0100, Mirsad Todorovac wrote:
...
>  141 static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>  142                                         struct kernfs_node *kn_from,
>  143                                         char *buf, size_t buflen)
...
>  172         /* Calculate how many bytes we need for the rest */
>  173         for (i = depth_to - 1; i >= 0; i--) {
>  174                 for (kn = kn_to, j = 0; j < i; j++)
>  175                         kn = kn->parent;
>  176                 len += strscpy(buf + len, "/",
>  177                                len < buflen ? buflen - len : 0);
>  178                 len += strscpy(buf + len, kn->name,
>  179                                len < buflen ? buflen - len : 0);
>  180         }
>  181
>  182         return len;
>  183 }
...
> This is safe, for we see that in case of count == 0 strscpy() just like
> strlcpy() turns to a virtual NOP.

The conversion itself isn't dangerous but it changes the return value of the
function. The comment is not updated and the callers are still assuming that
the function returns full length when the buffer is too short. e.g. Take a
look at cgroup_show_path(). All those paths seem safe but the code is more
more confusing because the conversions are half-way. I'm not necessarily
against the conversion but the benefit to risk / churn ratio doesn't seem
too attractive. If you wanna push this through, please make the conversion
complete including the comments and the callers and include a short summary
of the changes and why they're safe in the commit message.

Thanks.

-- 
tejun