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 > Cc: Christopher Bazley > Signed-off-by: Alejandro Colomar > --- > 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 > --