On Sat, Jul 05, 2025 at 10:33:53PM +0200, Alejandro Colomar wrote: > While doing this, I detected some anomalies in the existing code: > > mm/kfence/kfence_test.c: > > The last call to scnprintf() did increment 'cur', but it's > unused after that, so it was dead code. I've removed the dead > code in this patch. > > mm/mempolicy.c: > > This file uses the 'p += snprintf()' anti-pattern. That will > overflow the pointer on truncation, which has undefined > behavior. Using seprintf(), this bug is fixed. > > As in the previous file, here there was also dead code in the > last scnprintf() call, by incrementing a pointer that is not > used after the call. I've removed the dead code. > > mm/page_owner.c: > > Within print_page_owner(), there are some calls to scnprintf(), > which do report truncation. And then there are other calls to
This is a typo; I meant s/do/don't/
> snprintf(), where we handle errors (there are two 'goto err').
>
> I've kept the existing error handling, as I trust it's there for
> a good reason (i.e., we may want to avoid calling
> print_page_owner_memcg() if we truncated before). Please review
> if this amount of error handling is the right one, or if we want
> to add or remove some. For seprintf(), a single test for null
> after the last call is enough to detect truncation.
>
> mm/slub.c:
>
> Again, the 'p += snprintf()' anti-pattern. This is UB, and by
> using seprintf() we've fixed the bug.
>
> Cc: Kees Cook <k...@kernel.org>
> Cc: Christopher Bazley <chris.bazley.w...@gmail.com>
> Signed-off-by: Alejandro Colomar <a...@kernel.org>
> ---
> mm/kfence/kfence_test.c | 24 ++++++++++++------------
> mm/kmsan/kmsan_test.c | 4 ++--
> mm/mempolicy.c | 18 +++++++++---------
> mm/page_owner.c | 32 +++++++++++++++++---------------
> mm/slub.c | 5 +++--
> 5 files changed, 43 insertions(+), 40 deletions(-)
>
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index 00034e37bc9f..ff734c514c03 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -113,26 +113,26 @@ static bool report_matches(const struct expect_report
> *r)
> end = &expect[0][sizeof(expect[0]) - 1];
> switch (r->type) {
> case KFENCE_ERROR_OOB:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: out-of-bounds
> %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_UAF:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: use-after-free
> %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: use-after-free %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_CORRUPTION:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: memory
> corruption");
> + cur = seprintf(cur, end, "BUG: KFENCE: memory corruption");
> break;
> case KFENCE_ERROR_INVALID:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: invalid %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_INVALID_FREE:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid free");
> + cur = seprintf(cur, end, "BUG: KFENCE: invalid free");
> break;
> }
>
> - scnprintf(cur, end - cur, " in %pS", r->fn);
> + seprintf(cur, end, " in %pS", r->fn);
> /* The exact offset won't match, remove it; also strip module name. */
> cur = strchr(expect[0], '+');
> if (cur)
> @@ -144,26 +144,26 @@ static bool report_matches(const struct expect_report
> *r)
>
> switch (r->type) {
> case KFENCE_ERROR_OOB:
> - cur += scnprintf(cur, end - cur, "Out-of-bounds %s at",
> get_access_type(r));
> + cur = seprintf(cur, end, "Out-of-bounds %s at",
> get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_UAF:
> - cur += scnprintf(cur, end - cur, "Use-after-free %s at",
> get_access_type(r));
> + cur = seprintf(cur, end, "Use-after-free %s at",
> get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_CORRUPTION:
> - cur += scnprintf(cur, end - cur, "Corrupted memory at");
> + cur = seprintf(cur, end, "Corrupted memory at");
> break;
> case KFENCE_ERROR_INVALID:
> - cur += scnprintf(cur, end - cur, "Invalid %s at",
> get_access_type(r));
> + cur = seprintf(cur, end, "Invalid %s at", get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_INVALID_FREE:
> - cur += scnprintf(cur, end - cur, "Invalid free of");
> + cur = seprintf(cur, end, "Invalid free of");
> break;
> }
>
> - cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);
> + seprintf(cur, end, " 0x%p", (void *)addr);
>
> spin_lock_irqsave(&observed.lock, flags);
> if (!report_available())
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index 9733a22c46c1..a062a46b2d24 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -107,9 +107,9 @@ static bool report_matches(const struct expect_report *r)
> cur = expected_header;
> end = &expected_header[sizeof(expected_header) - 1];
>
> - cur += scnprintf(cur, end - cur, "BUG: KMSAN: %s", r->error_type);
> + cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
>
> - scnprintf(cur, end - cur, " in %s", r->symbol);
> + seprintf(cur, end, " in %s", r->symbol);
> /* The exact offset won't match, remove it; also strip module name. */
> cur = strchr(expected_header, '+');
> if (cur)
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b28a1e6ae096..c696e4a6f4c2 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3359,6 +3359,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
> void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> {
> char *p = buffer;
> + char *e = buffer + maxlen;
> nodemask_t nodes = NODE_MASK_NONE;
> unsigned short mode = MPOL_DEFAULT;
> unsigned short flags = 0;
> @@ -3384,33 +3385,32 @@ void mpol_to_str(char *buffer, int maxlen, struct
> mempolicy *pol)
> break;
> default:
> WARN_ON_ONCE(1);
> - snprintf(p, maxlen, "unknown");
> + seprintf(p, e, "unknown");
> return;
> }
>
> - p += snprintf(p, maxlen, "%s", policy_modes[mode]);
> + p = seprintf(p, e, "%s", policy_modes[mode]);
>
> if (flags & MPOL_MODE_FLAGS) {
> - p += snprintf(p, buffer + maxlen - p, "=");
> + p = seprintf(p, e, "=");
>
> /*
> * Static and relative are mutually exclusive.
> */
> if (flags & MPOL_F_STATIC_NODES)
> - p += snprintf(p, buffer + maxlen - p, "static");
> + p = seprintf(p, e, "static");
> else if (flags & MPOL_F_RELATIVE_NODES)
> - p += snprintf(p, buffer + maxlen - p, "relative");
> + p = seprintf(p, e, "relative");
>
> if (flags & MPOL_F_NUMA_BALANCING) {
> if (!is_power_of_2(flags & MPOL_MODE_FLAGS))
> - p += snprintf(p, buffer + maxlen - p, "|");
> - p += snprintf(p, buffer + maxlen - p, "balancing");
> + p = seprintf(p, e, "|");
> + p = seprintf(p, e, "balancing");
> }
> }
>
> if (!nodes_empty(nodes))
> - p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
> - nodemask_pr_args(&nodes));
> + seprintf(p, e, ":%*pbl", nodemask_pr_args(&nodes));
> }
>
> #ifdef CONFIG_SYSFS
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index cc4a6916eec6..5811738e3320 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -496,7 +496,7 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
> /*
> * Looking for memcg information and print it out
> */
> -static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> +static inline char *print_page_owner_memcg(char *p, const char end[0],
> struct page *page)
> {
> #ifdef CONFIG_MEMCG
> @@ -511,8 +511,7 @@ static inline int print_page_owner_memcg(char *kbuf,
> size_t count, int ret,
> goto out_unlock;
>
> if (memcg_data & MEMCG_DATA_OBJEXTS)
> - ret += scnprintf(kbuf + ret, count - ret,
> - "Slab cache page\n");
> + p = seprintf(p, end, "Slab cache page\n");
>
> memcg = page_memcg_check(page);
> if (!memcg)
> @@ -520,7 +519,7 @@ static inline int print_page_owner_memcg(char *kbuf,
> size_t count, int ret,
>
> online = (memcg->css.flags & CSS_ONLINE);
> cgroup_name(memcg->css.cgroup, name, sizeof(name));
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, end,
> "Charged %sto %smemcg %s\n",
> PageMemcgKmem(page) ? "(via objcg) " : "",
> online ? "" : "offline ",
> @@ -529,7 +528,7 @@ static inline int print_page_owner_memcg(char *kbuf,
> size_t count, int ret,
> rcu_read_unlock();
> #endif /* CONFIG_MEMCG */
>
> - return ret;
> + return p;
> }
>
> static ssize_t
> @@ -538,14 +537,16 @@ print_page_owner(char __user *buf, size_t count,
> unsigned long pfn,
> depot_stack_handle_t handle)
> {
> int ret, pageblock_mt, page_mt;
> - char *kbuf;
> + char *kbuf, *p, *e;
>
> count = min_t(size_t, count, PAGE_SIZE);
> kbuf = kmalloc(count, GFP_KERNEL);
> if (!kbuf)
> return -ENOMEM;
>
> - ret = scnprintf(kbuf, count,
> + p = kbuf;
> + e = kbuf + count;
> + p = seprintf(p, e,
> "Page allocated via order %u, mask %#x(%pGg), pid %d,
> tgid %d (%s), ts %llu ns\n",
> page_owner->order, page_owner->gfp_mask,
> &page_owner->gfp_mask, page_owner->pid,
> @@ -555,7 +556,7 @@ print_page_owner(char __user *buf, size_t count, unsigned
> long pfn,
> /* Print information relevant to grouping pages by mobility */
> pageblock_mt = get_pageblock_migratetype(page);
> page_mt = gfp_migratetype(page_owner->gfp_mask);
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, e,
> "PFN 0x%lx type %s Block %lu type %s Flags %pGp\n",
> pfn,
> migratetype_names[page_mt],
> @@ -563,22 +564,23 @@ print_page_owner(char __user *buf, size_t count,
> unsigned long pfn,
> migratetype_names[pageblock_mt],
> &page->flags);
>
> - ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
> - if (ret >= count)
> - goto err;
> + p = stack_depot_seprint(handle, p, e, 0);
> + if (p == NULL)
> + goto err; // XXX: Should we remove this error handling?
>
> if (page_owner->last_migrate_reason != -1) {
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, e,
> "Page has been migrated, last migrate reason: %s\n",
> migrate_reason_names[page_owner->last_migrate_reason]);
> }
>
> - ret = print_page_owner_memcg(kbuf, count, ret, page);
> + p = print_page_owner_memcg(p, e, page);
>
> - ret += snprintf(kbuf + ret, count - ret, "\n");
> - if (ret >= count)
> + p = seprintf(p, e, "\n");
> + if (p == NULL)
> goto err;
>
> + ret = p - kbuf;
> if (copy_to_user(buf, kbuf, ret))
> ret = -EFAULT;
>
> diff --git a/mm/slub.c b/mm/slub.c
> index be8b09e09d30..b67c6ca0d0f7 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -7451,6 +7451,7 @@ static char *create_unique_id(struct kmem_cache *s)
> {
> char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
> char *p = name;
> + char *e = name + ID_STR_LENGTH;
>
> if (!name)
> return ERR_PTR(-ENOMEM);
> @@ -7475,9 +7476,9 @@ static char *create_unique_id(struct kmem_cache *s)
> *p++ = 'A';
> if (p != name + 1)
> *p++ = '-';
> - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);
> + p = seprintf(p, e, "%07u", s->size);
>
> - if (WARN_ON(p > name + ID_STR_LENGTH - 1)) {
> + if (WARN_ON(p == NULL)) {
> kfree(name);
> return ERR_PTR(-EINVAL);
> }
> --
> 2.50.0
>
--
<https://www.alejandro-colomar.es/>
signature.asc
Description: PGP signature
