strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated.
Since strscpy() returns -E2BIG on truncate, we rely on
strlen(src) to imitate strlcpy behavior.
Signed-off-by: Azeem Shaikh <[email protected]>
---
fs/kernfs/dir.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 45b6919903e6..0f46d7b304b0 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -51,12 +51,19 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
#endif
}
+/* strscpy_mock_strlcpy - imitates strlcpy API but uses strscpy underneath. */
+static size_t strscpy_mock_strlcpy(char *dest, const char *src, size_t count)
+{
+ strscpy(dest, src, count);
+ return strlen(src);
+}
+
static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
{
if (!kn)
- return strlcpy(buf, "(null)", buflen);
+ return strscpy_mock_strlcpy(buf, "(null)", buflen);
- return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
+ return strscpy_mock_strlcpy(buf, kn->parent ? kn->name : "/", buflen);
}
/* kernfs_node_depth - compute depth from @from to @to */
@@ -141,13 +148,13 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
int i, j;
if (!kn_to)
- return strlcpy(buf, "(null)", buflen);
+ return strscpy_mock_strlcpy(buf, "(null)", buflen);
if (!kn_from)
kn_from = kernfs_root(kn_to)->kn;
if (kn_from == kn_to)
- return strlcpy(buf, "/", buflen);
+ return strscpy_mock_strlcpy(buf, "/", buflen);
common = kernfs_common_ancestor(kn_from, kn_to);
if (WARN_ON(!common))
@@ -159,16 +166,16 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
buf[0] = '\0';
for (i = 0; i < depth_from; i++)
- len += strlcpy(buf + len, parent_str,
+ len += strscpy_mock_strlcpy(buf + len, parent_str,
len < buflen ? buflen - len : 0);
/* Calculate how many bytes we need for the rest */
for (i = depth_to - 1; i >= 0; i--) {
for (kn = kn_to, j = 0; j < i; j++)
kn = kn->parent;
- len += strlcpy(buf + len, "/",
+ len += strscpy_mock_strlcpy(buf + len, "/",
len < buflen ? buflen - len : 0);
- len += strlcpy(buf + len, kn->name,
+ len += strscpy_mock_strlcpy(buf + len, kn->name,
len < buflen ? buflen - len : 0);
}
@@ -851,7 +858,7 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
spin_lock_irq(&kernfs_pr_cont_lock);
- len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
+ len = strscpy_mock_strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
if (len >= sizeof(kernfs_pr_cont_buf)) {
spin_unlock_irq(&kernfs_pr_cont_lock);
--
2.40.1.521.gf1e218fcd8-goog
On Tue, May 09, 2023 at 08:40:08PM -0400, Azeem Shaikh wrote:
> Thanks for the quick response Tejun! I forgot to include the relevant links
> in the commit log, sorry about that.
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> mentions that strlcpy is deprecated and should be replaced with strscpy
> instead. https://github.com/KSPP/linux/issues/89 is the relevant security
> bug which aims to remove strlcpy from the kernel entirely.
>
> Does that help justify this change? And if so, would you like me to resend
> this patch with the relevant links included in the commit log?
Yeah, I think so. Please explicitly state that this is a part of tree-wide
cleanup.
Thanks.
--
tejun
On Tue, May 9, 2023 at 5:30 PM Azeem Shaikh <[email protected]> wrote:
>
> +/* strscpy_mock_strlcpy - imitates strlcpy API but uses strscpy underneath. */
> +static size_t strscpy_mock_strlcpy(char *dest, const char *src, size_t count)
> +{
> + strscpy(dest, src, count);
> + return strlen(src);
Absolutely not.
This makes the whole exercise pointless.
The reason to use strscpy() is to *avoid* doing the strlen() on the
source, and limit things to the limited size.
If you need to do the strlen(), then use strlcpy(). It's a broken
interface, but creating this kind of horror wrapper that does the same
thing as strlcpy() is worse than just using the regular version.
So the strscpy() conversion should *only* be done if the caller
doesn't care about the difference in return values (or done *together*
with changing the caller to use the nicer strscpy() return value).
It's also worth noting that 'strscpy()' not only returns a negative
error value when the string doesn't fit - it will also possibly do the
copy one word at a time, and may write extra zeroes at the end of the
destination (all within the given size, of course).
So strscpy() is _different_ from strlcpy(), and the conversion should
not be done unless those differences are ok.
Linus
> Absolutely not.
>
> This makes the whole exercise pointless.
>
> The reason to use strscpy() is to *avoid* doing the strlen() on the
> source, and limit things to the limited size.
>
> If you need to do the strlen(), then use strlcpy(). It's a broken
> interface, but creating this kind of horror wrapper that does the same
> thing as strlcpy() is worse than just using the regular version.
>
> So the strscpy() conversion should *only* be done if the caller
> doesn't care about the difference in return values (or done *together*
> with changing the caller to use the nicer strscpy() return value).
>
> It's also worth noting that 'strscpy()' not only returns a negative
> error value when the string doesn't fit - it will also possibly do the
> copy one word at a time, and may write extra zeroes at the end of the
> destination (all within the given size, of course).
>
> So strscpy() is _different_ from strlcpy(), and the conversion should
> not be done unless those differences are ok.
Thanks Linus, that helps clarify a lot. I traced the usage of these
functions across the kernel and plan to do direct replacement only
where it's safe (see thread here:
https://lore.kernel.org/all/CADmuW3XiYpGK7BessXJWjGnnxZti_3mawDSX7QPawsfmATxCng@mail.gmail.com/).
Let me know if that works for you.