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

[Christophe JAILLET]

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;
>   	}

...