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
| ||
|
Message-ID: <40b65db9-1b37-45b6-8afe-7be2df11cfa9@wanadoo.fr> 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