| 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
| ||
|
Date: Thu, 30 Nov 2023 21:38:11 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Kees Cook <keescook@...omium.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>,
Johannes Weiner <hannes@...xchg.org>, Waiman Long <longman@...hat.com>,
cgroups@...r.kernel.org, Azeem Shaikh <azeemshaikh38@...il.com>,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 3/3] kernfs: Convert kernfs_path_from_node_locked()
from strlcpy() to strscpy()
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@...uxfoundation.org>
> Cc: Tejun Heo <tj@...nel.org>
> Cc: Zefan Li <lizefan.x@...edance.com>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Waiman Long <longman@...hat.com>
> Cc: cgroups@...r.kernel.org
> Co-developed-by: Azeem Shaikh <azeemshaikh38@...il.com>
> Signed-off-by: Azeem Shaikh <azeemshaikh38@...il.com>
> Link: https://lore.kernel.org/r/20231116192127.1558276-3-keescook@chromium.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> 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;
> }
...
Powered by blists - more mailing lists