[RFC PATCH v1 1/1] kernfs: replace deprecated strlcpy() with the safer strscpy()

[Mirsad Goran Todorovac]

According to strlcpy() being officially deprecated and the encouragement
to remove the remaining occurrences, this came as the intriguing example.

We don't have a semantic way to bail out in the case kn->name
doesn't fit into buflen sized buffer which traces to sizeof(kernfs_pr_cont_buf)
which is PATH_MAX or currently 4096 bytes, so emulate what would
strlcpy(buf, kn->name, buflen) do not to break things.

(It would not leave an empty string in buf. But this defeats the purpose that
overrun is not silently ignored, but currently there doesn't seem a way to
signal the buffer overrun error to the caller. As the function is static and
the buf is only printed, it seems prudent to leave truncated path rather than
an empty string.)

FIXME: strlen(kn->name) is unprotected from overrun, but for now we preserve
exact behavior using strncpy(). As the return value passed by kernfs_name()
to pr_cont_kernfs_name() where it is ignored later in the code, we could as
well return the number of actually copied characters

     return buflen - 1;

rather than pointless and dangerous strlen(kn->name) of strlcpy(buf, kn->name, buflen).

As semantics are not agreed upon, a FIXME comment was left.

The strlcpy() in kernfs_path_from_node_locked() can never overrun as "(null)"
is needless to say safe for PATH_MAX sized buf and the rest is carefully
computed and protected, so translation is one-for-one.

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@xxxxxxxxx>
Cc: Aditya Kali <Tejun Heo <tj@xxxxxxxxxx>adityakali@xxxxxxxxxx>
Cc: Tejun Heo <tj@xxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Link: https://lwn.net/ml/ksummit-discuss/20231019054642.GF14346@xxxxxx/#t
Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
---
v1:
 initial proposal for substituting deprecated strlcpy() with the preferred strscpy()

 fs/kernfs/dir.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 8b2bd65d70e7..87456f3842c0 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -53,10 +53,32 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
 
 static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
 {
-	if (!kn)
-		return strlcpy(buf, "(null)", buflen);
+	int len;
 
-	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
+	if (!kn)
+		return strscpy(buf, "(null)", buflen);
+
+	/**
+	 * We don't have a semantic way to bail out in the case kn->name
+	 * doesn't fit into the buflen which traces to sizeof(kernfs_pr_cont_buf)
+	 * which is PATH_MAX or currently 4096 bytes, so emulate what would
+	 * strlcpy() do not to break things.
+	 *
+	 * FIXME: strlen(kn->name) is unprotected from overrun, but for now we preserve
+	 * exact behavior using strncpy(). As the return value is ignored later in the
+	 * code, we could as well return
+	 *
+	 *	return buflen - 1;
+	 *
+	 * rather than pointless and dangerous strlen(kn->name) of
+	 * strlcpy(buf, kn->name, buflen).
+	 */
+	if ((len = strscpy(buf, kn->parent ? kn->name : "/", buflen)) == -E2BIG) {
+		strncpy(buf, kn->name, buflen);
+		buf[buflen - 1] = '\0';
+		return buflen - 1;
+	} else
+		return len;
 }
 
 /* kernfs_node_depth - compute depth from @from to @to */
@@ -141,13 +163,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 +181,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);
 	}
 
@@ -850,16 +872,13 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
 					  const unsigned char *path,
 					  const void *ns)
 {
-	size_t len;
 	char *p, *name;
 
 	lockdep_assert_held_read(&kernfs_root(parent)->kernfs_rwsem);
 
 	spin_lock_irq(&kernfs_pr_cont_lock);
 
-	len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
-
-	if (len >= sizeof(kernfs_pr_cont_buf)) {
+	if (strscpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf)) == -E2BIG) {
 		spin_unlock_irq(&kernfs_pr_cont_lock);
 		return NULL;
 	}
-- 
2.34.1