Skip to content

Commit ff6d413

Browse files
keesgregkh
authored andcommitted
kernfs: Convert kernfs_path_from_node_locked() from strlcpy() to strscpy()
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 <[email protected]> Cc: Tejun Heo <[email protected]> Cc: Zefan Li <[email protected]> Cc: Johannes Weiner <[email protected]> Cc: Waiman Long <[email protected]> Cc: <[email protected]> Co-developed-by: Azeem Shaikh <[email protected]> Signed-off-by: Azeem Shaikh <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Kees Cook <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 5b56bf5 commit ff6d413

File tree

4 files changed

+21
-21
lines changed

4 files changed

+21
-21
lines changed

fs/kernfs/dir.c

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
127127
*
128128
* [3] when @kn_to is %NULL result will be "(null)"
129129
*
130-
* Return: the length of the full path. If the full length is equal to or
130+
* Return: the length of the constructed path. If the path would have been
131131
* greater than @buflen, @buf contains the truncated path with the trailing
132132
* '\0'. On error, -errno is returned.
133133
*/
@@ -138,16 +138,17 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
138138
struct kernfs_node *kn, *common;
139139
const char parent_str[] = "/..";
140140
size_t depth_from, depth_to, len = 0;
141+
ssize_t copied;
141142
int i, j;
142143

143144
if (!kn_to)
144-
return strlcpy(buf, "(null)", buflen);
145+
return strscpy(buf, "(null)", buflen);
145146

146147
if (!kn_from)
147148
kn_from = kernfs_root(kn_to)->kn;
148149

149150
if (kn_from == kn_to)
150-
return strlcpy(buf, "/", buflen);
151+
return strscpy(buf, "/", buflen);
151152

152153
common = kernfs_common_ancestor(kn_from, kn_to);
153154
if (WARN_ON(!common))
@@ -158,18 +159,19 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
158159

159160
buf[0] = '\0';
160161

161-
for (i = 0; i < depth_from; i++)
162-
len += strlcpy(buf + len, parent_str,
163-
len < buflen ? buflen - len : 0);
162+
for (i = 0; i < depth_from; i++) {
163+
copied = strscpy(buf + len, parent_str, buflen - len);
164+
if (copied < 0)
165+
return copied;
166+
len += copied;
167+
}
164168

165169
/* Calculate how many bytes we need for the rest */
166170
for (i = depth_to - 1; i >= 0; i--) {
167171
for (kn = kn_to, j = 0; j < i; j++)
168172
kn = kn->parent;
169-
len += strlcpy(buf + len, "/",
170-
len < buflen ? buflen - len : 0);
171-
len += strlcpy(buf + len, kn->name,
172-
len < buflen ? buflen - len : 0);
173+
174+
len += scnprintf(buf + len, buflen - len, "/%s", kn->name);
173175
}
174176

175177
return len;
@@ -214,7 +216,7 @@ int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
214216
* path (which includes '..'s) as needed to reach from @from to @to is
215217
* returned.
216218
*
217-
* Return: the length of the full path. If the full length is equal to or
219+
* Return: the length of the constructed path. If the path would have been
218220
* greater than @buflen, @buf contains the truncated path with the trailing
219221
* '\0'. On error, -errno is returned.
220222
*/
@@ -265,12 +267,10 @@ void pr_cont_kernfs_path(struct kernfs_node *kn)
265267
sz = kernfs_path_from_node(kn, NULL, kernfs_pr_cont_buf,
266268
sizeof(kernfs_pr_cont_buf));
267269
if (sz < 0) {
268-
pr_cont("(error)");
269-
goto out;
270-
}
271-
272-
if (sz >= sizeof(kernfs_pr_cont_buf)) {
273-
pr_cont("(name too long)");
270+
if (sz == -E2BIG)
271+
pr_cont("(name too long)");
272+
else
273+
pr_cont("(error)");
274274
goto out;
275275
}
276276

kernel/cgroup/cgroup-v1.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,7 @@ void cgroup1_release_agent(struct work_struct *work)
802802
goto out_free;
803803

804804
ret = cgroup_path_ns(cgrp, pathbuf, PATH_MAX, &init_cgroup_ns);
805-
if (ret < 0 || ret >= PATH_MAX)
805+
if (ret < 0)
806806
goto out_free;
807807

808808
argv[0] = agentbuf;

kernel/cgroup/cgroup.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1893,7 +1893,7 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node,
18931893
len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX);
18941894
spin_unlock_irq(&css_set_lock);
18951895

1896-
if (len >= PATH_MAX)
1896+
if (len == -E2BIG)
18971897
len = -ERANGE;
18981898
else if (len > 0) {
18991899
seq_escape(sf, buf, " \t\n\\");
@@ -6278,7 +6278,7 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
62786278
if (cgroup_on_dfl(cgrp) || !(tsk->flags & PF_EXITING)) {
62796279
retval = cgroup_path_ns_locked(cgrp, buf, PATH_MAX,
62806280
current->nsproxy->cgroup_ns);
6281-
if (retval >= PATH_MAX)
6281+
if (retval == -E2BIG)
62826282
retval = -ENAMETOOLONG;
62836283
if (retval < 0)
62846284
goto out_unlock;

kernel/cgroup/cpuset.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4941,7 +4941,7 @@ int proc_cpuset_show(struct seq_file *m, struct pid_namespace *ns,
49414941
retval = cgroup_path_ns(css->cgroup, buf, PATH_MAX,
49424942
current->nsproxy->cgroup_ns);
49434943
css_put(css);
4944-
if (retval >= PATH_MAX)
4944+
if (retval == -E2BIG)
49454945
retval = -ENAMETOOLONG;
49464946
if (retval < 0)
49474947
goto out_free;

0 commit comments

Comments
 (0)