Re: [PATCH v2 3/3] kernfs: Convert kernfs_path_from_node_locked() from strlcpy() to strscpy()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 30/11/2023 à 21:12, Kees Cook a écrit :
One of the last remaining users of strlcpy() in the kernel is
kernfs_path_from_node_locked(), which passes back the problematic "length
we _would_ have copied" return value to indicate truncation.  Convert the
chain of all callers to use the negative return value (some of which
already doing this explicitly). All callers were already also checking
for negative return values, so the risk to missed checks looks very low.

In this analysis, it was found that cgroup1_release_agent() actually
didn't handle the "too large" condition, so this is technically also a
bug fix. :)

Here's the chain of callers, and resolution identifying each one as now
handling the correct return value:

kernfs_path_from_node_locked()
         kernfs_path_from_node()
                 pr_cont_kernfs_path()
                         returns void
                 kernfs_path()
                         sysfs_warn_dup()
                                 return value ignored
                         cgroup_path()
                                 blkg_path()
                                         bfq_bic_update_cgroup()
                                                 return value ignored
                                 TRACE_IOCG_PATH()
                                         return value ignored
                                 TRACE_CGROUP_PATH()
                                         return value ignored
                                 perf_event_cgroup()
                                         return value ignored
                                 task_group_path()
                                         return value ignored
                                 damon_sysfs_memcg_path_eq()
                                         return value ignored
                                 get_mm_memcg_path()
                                         return value ignored
                                 lru_gen_seq_show()
                                         return value ignored
                         cgroup_path_from_kernfs_id()
                                 return value ignored
                 cgroup_show_path()
                         already converted "too large" error to negative value
                 cgroup_path_ns_locked()
                         cgroup_path_ns()
                                 bpf_iter_cgroup_show_fdinfo()
                                         return value ignored
                                 cgroup1_release_agent()
                                         wasn't checking "too large" error
                         proc_cgroup_show()
                                 already converted "too large" to negative value

Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Tejun Heo <tj@xxxxxxxxxx>
Cc: Zefan Li <lizefan.x@xxxxxxxxxxxxx>
Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
Cc: Waiman Long <longman@xxxxxxxxxx>
Cc: cgroups@xxxxxxxxxxxxxxx
Co-developed-by: Azeem Shaikh <azeemshaikh38@xxxxxxxxx>
Signed-off-by: Azeem Shaikh <azeemshaikh38@xxxxxxxxx>
Link: https://lore.kernel.org/r/20231116192127.1558276-3-keescook@xxxxxxxxxxxx
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
  fs/kernfs/dir.c           | 37 ++++++++++++++++++++-----------------
  kernel/cgroup/cgroup-v1.c |  2 +-
  kernel/cgroup/cgroup.c    |  4 ++--
  kernel/cgroup/cpuset.c    |  2 +-
  4 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 8c0e5442597e..183f353b3852 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -127,7 +127,7 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
   *
   * [3] when @kn_to is %NULL result will be "(null)"
   *
- * Return: the length of the full path.  If the full length is equal to or
+ * Return: the length of the constructed path.  If the path would have been
   * greater than @buflen, @buf contains the truncated path with the trailing
   * '\0'.  On error, -errno is returned.
   */
@@ -138,16 +138,17 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
  	struct kernfs_node *kn, *common;
  	const char parent_str[] = "/..";
  	size_t depth_from, depth_to, len = 0;
+	ssize_t copied;
  	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))
@@ -158,18 +159,22 @@ 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 < buflen ? buflen - len : 0);
+	for (i = 0; i < depth_from; i++) {
+		copied = strscpy(buf + len, parent_str, buflen - len);
+		if (copied < 0)
+			return copied;
+		len += copied;
+	}
/* 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 < buflen ? buflen - len : 0);
-		len += strlcpy(buf + len, kn->name,
-			       len < buflen ? buflen - len : 0);
+
+		copied = scnprintf(buf + len, buflen - len, "/%s", kn->name);
+		if (copied < 0)

Can scnprintf() return <0 ?

+			return copied;
+		len += copied;
  	}

...





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux