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

Message-Id: <20231122212008.11790-1-mirsad.todorovac@alu.unizg.hr>
Date:   Wed, 22 Nov 2023 22:20:10 +0100
From:   Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To:     linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tejun Heo <tj@...nel.org>,
        Mirsad Todorovac <mirsad.todorovac@....unizg.hr>,
        Konstantin Khlebnikov <koct9i@...il.com>,
        Aditya Kali <adityakali@...gle.com>
Subject: [RFC PATCH v2 1/1] kernfs: replace deprecated strlcpy() with strscpy()

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().

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

Fixes: 17627157cda13 ("kernfs: handle null pointers while printing node name and path")
Fixes: 3eef34ad7dc36 ("kernfs: implement kernfs_get_parent(), kernfs_name/path() and friends")
Fixes: 9f6df573a4041 ("kernfs: Add API to generate relative kernfs path")
Fixes: 3abb1d90f5d93 ("kernfs: make kernfs_path*() behave in the style of strlcpy()")
Fixes: e56ed358afd81 ("kernfs: make kernfs_walk_ns() use kernfs_pr_cont_buf[]")
Cc: Konstantin Khlebnikov <koct9i@...il.com>
Cc: Aditya Kali <adityakali@...gle.com>
Cc: Tejun Heo <tj@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org
Link: https://lwn.net/ml/ksummit-discuss/20231019054642.GF14346@lst.de/#t
Link: https://lwn.net/Articles/659214/
Signed-off-by: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
---
v2:
 Fixed a Cc: address bug.
 Fixing the patch according to the new definition of strscpy() in
    LWN article https://lwn.net/Articles/659214/ that makes strscpy_truncate()
    obsoleted.

 fs/kernfs/dir.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 8b2bd65d70e7..a6971a6756bc 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -53,10 +53,17 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
 
 static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
 {
+	size_t len;
+
 	if (!kn)
-		return strlcpy(buf, "(null)", buflen);
+		return strscpy(buf, "(null)", buflen);
+
+	len = strscpy(buf, kn->parent ? kn->name : "/", buflen);
 
-	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
+	if (unlikely(len == -E2BIG)) {
+		return buflen - 1;
+	} else
+		return len;
 }
 
 /* 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(buf, "(null)", buflen);
 
 	if (!kn_from)
 		kn_from = kernfs_root(kn_to)->kn;
 
 	if (kn_from == kn_to)
-		return strlcpy(buf, "/", buflen);
+		return strscpy(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(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(buf + len, "/",
 			       len < buflen ? buflen - len : 0);
-		len += strlcpy(buf + len, kn->name,
+		len += strscpy(buf + len, kn->name,
 			       len < buflen ? buflen - len : 0);
 	}
 
@@ -857,9 +864,9 @@ 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(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
 
-	if (len >= sizeof(kernfs_pr_cont_buf)) {
+	if (unlikely(len == -E2BIG)) {
 		spin_unlock_irq(&kernfs_pr_cont_lock);
 		return NULL;
 	}
-- 
2.34.1