mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
@ 2026-07-21 12:52 Oleg Deomi
  2026-07-21 23:38 ` Andrew Morton
  2026-07-23 13:04 ` Bradley Morgan
  0 siblings, 2 replies; 8+ messages in thread
From: Oleg Deomi @ 2026-07-21 12:52 UTC (permalink / raw)
  To: Balbir Singh; +Cc: linux-kernel, Andrew Morton


[-- Attachment #1.1: Type: text/plain, Size: 6221 bytes --]

TASKSTATS REGISTER_CPUMASK silently drops the last byte of the cpumask
string it's given, due to a destination-buffer-sizing bug in
kernel/taskstats.c's parse(). A request for "0-31" is silently parsed as
"0-3" instead — the call still succeeds (ACK, no error), so the caller
has no way to detect that most of the CPUs it asked to cover were never
actually registered.

AFFECTED KERNEL
    Confirmed on: 7.1.3-200.fc44.x86_64 (Fedora 44), bare metal, x86_64,
    32 logical CPUs (AMD Ryzen 9 9950X3D).
    Same buggy code confirmed present, by direct source read, in the
    linux-7.1.4 tree at the line numbers cited below — this is not
    version-specific to 7.1.3, it's the current upstream code.

ROOT CAUSE

kernel/taskstats.c, parse():

    len = nla_len(na);                 /* exact string length, e.g. 4 for
"0-31" */
    data = kmalloc(len, GFP_KERNEL);   /* allocates exactly `len` bytes */
    nla_strscpy(data, na, len);        /* dstsize == len — return value
IGNORED */
    ret = cpulist_parse(data, mask);

lib/nlattr.c, nla_strscpy()'s own documented contract: "Copies at most
dstsize - 1 bytes into the destination buffer... Return: srclen ... -E2BIG
- If ... @nla length greater than @dstsize." Its actual behavior for this
call site:

    srclen = nla_len(nla);             /* 4 */
    if (srclen > 0 && src[srclen-1] == '\0')
        srclen--;                      /* no-op: no trailing NUL was sent */
    if (srclen >= dstsize) {           /* 4 >= 4 -> true, ALWAYS true here
*/
        len = dstsize - 1;             /* 3 */
        ret = -E2BIG;                  /* parse() never checks this */
    }
    memcpy(dst, src, len);             /* copies "0-3", drops the "1" */
    return ret;                        /* -E2BIG, discarded */

parse() passes dstsize == nla_len(na) — the attribute's exact length,
with zero bytes of headroom. nla_strscpy() always needs at least one
byte of headroom to write its own trailing NUL, so passing dstsize ==
srclen makes the truncation branch unconditional. Every REGISTER_CPUMASK
call loses its last character, silently.

Note: TASKSTATS_CMD_ATTR_REGISTER_CPUMASK's policy entry declares
`.type = NLA_STRING`, not NLA_NUL_STRING — per include/net/netlink.h's
own documented distinction ("NLA_STRING: Maximum length of string" vs
"NLA_NUL_STRING: Maximum length of string (excluding NUL)"), callers are
not required or expected to send a trailing NUL for this attribute type.
This is a bug in parse()'s own buffer sizing, not a caller error.

REPRODUCTION

    sudo python3 reproduce_minimal.py [exits_per_cpu] [duration_seconds]

Test system requirement: at least 11 CPUs online (below that, the same
bug fails registration outright with EINVAL instead of silently
truncating the range — see reproduce_minimal.py's own startup check).

Self-contained, Python 3 stdlib only, no kernel-tracing tools, no
non-standard dependencies beyond `taskset` (util-linux). Registers a
TASKSTATS exit-push listener for "0-{N-1}" (every CPU on the host) twice
in a row against identical, controlled load (short-lived processes
explicitly pinned to every CPU via taskset): once with the string as any
normal caller would send it, once with an explicit trailing NUL byte
appended to the SAME logical request. Reports message counts side by
side.

Confirmed output on the affected host (32 CPUs, 10 exits/CPU, 15s per
phase):

    plain cpumask='0-31':             53 messages received
    same cpumask + trailing NUL byte: 406 messages received

(Exact counts vary run to run with ambient system load — the ~7-8x ratio
between the two phases is the reproducible part, not the absolute
numbers.)

Both phases register the exact same cpumask ("0-31") and receive the
exact same load. The only difference is one extra NUL byte in the
attribute payload — which should be semantically meaningless per the
NLA_STRING contract, and changes the delivered message count by ~7x.

EXPECTED BEHAVIOR
    REGISTER_CPUMASK("0-31") delivers TASKSTATS exit-push notifications
    for tasks exiting on any of CPUs 0 through 31.

ACTUAL BEHAVIOR
    REGISTER_CPUMASK("0-31") only ever delivers for CPUs 0-3 — the
    silently-truncated "0-3" is what's actually registered. Confirmed
    directly via kernel-side kprobe tracing (taskstats_exit() ->
    netlink_unicast()): CPUs 4-31 show zero delivery attempts, 100% of
    the time, across every CPU and hundreds of exits tested.

SEVERITY / IMPACT
    Silent, undetectable from userspace: the REGISTER_CPUMASK call
    itself always returns success. Any application built on this API
    (accounting/monitoring daemons, cgroup-adjacent tooling, etc.) that
    requests a CPU range whose string representation is long enough to
    trigger this (effectively any range beyond a single-digit endpoint —
    "0-9" happens to fail outright with EINVAL instead, since it
    truncates to the malformed "0-", but anything from "0-10" upward
    truncates to something that still parses, e.g. "0-1") receives
    exit-push data for only a small, silently-wrong subset of the CPUs
    it asked for, with no error, warning, or other observable signal.

    Also affects the documented "all" keyword the same way: "all" (3
    bytes) truncates to "al", an unrecognized token, which fails
    registration outright with EINVAL — this can look like "all" isn't
    supported on a given kernel at all, when it's actually this same bug.
    Verified: "all" + an explicit trailing NUL registers successfully
    and delivers to 100% of CPUs.

SUGGESTED FIX DIRECTION
    parse() should allocate len + 1 bytes and pass len + 1 as
    nla_strscpy()'s dstsize, giving it the headroom its own contract
    requires; and/or parse() should check nla_strscpy()'s return value
    and propagate -E2BIG instead of silently discarding it. Have not
    prepared a formal patch — happy to if useful, wanted to report the
    finding with a minimal, verifiable reproduction first.

ATTACHMENT
    reproduce_minimal.py — the script referenced above. Plain stdlib
    Python 3, no project-specific code, run as shown.

[-- Attachment #1.2: Type: text/html, Size: 6996 bytes --]

[-- Attachment #2: reproduce_minimal.py --]
[-- Type: text/x-python, Size: 11301 bytes --]

#!/usr/bin/env python3
"""
Minimal, self-contained reproducer for a TASKSTATS REGISTER_CPUMASK bug.

BUG SUMMARY
    parse() in kernel/taskstats.c calls:

        len = nla_len(na);
        data = kmalloc(len, GFP_KERNEL);
        nla_strscpy(data, na, len);          /* dstsize == len, no headroom */
        ret = cpulist_parse(data, mask);

    nla_strscpy()'s own contract (lib/nlattr.c) is "copies at most
    dstsize - 1 bytes" and returns -E2BIG if the source is >= dstsize.
    Passing dstsize == nla_len(na) (the exact source length, zero
    headroom for nla_strscpy's own trailing NUL) makes the truncation
    branch unconditional: parse() ALWAYS silently drops the LAST BYTE of
    every REGISTER_CPUMASK string, and never checks the -E2BIG return
    that would say so. "0-31" becomes "0-3". This script demonstrates
    the practical consequence (most CPUs on a multi-core host silently
    never deliver TASKSTATS exit-push data at all) and a workaround
    (append an explicit trailing NUL byte to the attribute payload,
    which nla_strscpy special-cases and decrements srclen for BEFORE the
    length check, sidestepping the bug).

WHAT THIS SCRIPT DOES
    1. Registers a TASKSTATS exit-push listener for "0-{N-1}" (every CPU),
       WITHOUT a workaround. Spawns short-lived processes pinned to every
       CPU (taskset) and counts how many exit-push messages arrive.
    2. Deregisters, then re-registers the IDENTICAL cpumask string but
       with an explicit trailing NUL byte appended to the attribute
       payload. Repeats the same pinned-process load. Counts again.
    3. Prints both counts side by side. On an affected kernel, phase 2's
       count is dramatically higher (delivery from nearly every CPU,
       instead of only the lowest few).

REQUIREMENTS
    Linux, CAP_NET_ADMIN (run as root), `taskset` (util-linux, virtually
    always present), Python 3, stdlib only — no non-stdlib dependencies,
    no project-specific code.

USAGE
    sudo python3 reproduce_minimal.py [exits_per_cpu] [duration_seconds]
"""
import os
import socket
import struct
import subprocess
import sys
import time

NETLINK_GENERIC = 16
GENL_ID_CTRL = 0x10
CTRL_CMD_GETFAMILY = 3
CTRL_ATTR_FAMILY_NAME = 2
CTRL_ATTR_FAMILY_ID = 1
TASKSTATS_CMD_GET = 1
TASKSTATS_CMD_ATTR_REGISTER_CPUMASK = 3
TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK = 4
NLM_F_REQUEST = 0x01
NLM_F_ACK = 0x04
NLMSG_ERROR = 2
NLMSG_HDRLEN = 16
GENL_HDRLEN = 4
NLA_HDRLEN = 4


def nlattr(nla_type: int, data: bytes) -> bytes:
    raw_len = NLA_HDRLEN + len(data)
    pad = (-raw_len) % 4
    return struct.pack("HH", raw_len, nla_type) + data + b"\x00" * pad


def nlmsg(nlmsg_type: int, cmd: int, attrs: bytes, seq: int = 0, flags: int = NLM_F_REQUEST) -> bytes:
    payload = struct.pack("BBH", cmd, 1, 0) + attrs
    total_len = NLMSG_HDRLEN + len(payload)
    hdr = struct.pack("IHHII", total_len, nlmsg_type, flags, seq, 0)
    return hdr + payload


def parse_nlattrs(data: bytes) -> dict:
    attrs, off = {}, 0
    while off + NLA_HDRLEN <= len(data):
        nla_len, nla_type = struct.unpack_from("HH", data, off)
        if nla_len < NLA_HDRLEN:
            break
        attrs[nla_type] = data[off + NLA_HDRLEN: off + nla_len]
        off += (nla_len + 3) & ~3
    return attrs


def resolve_family(sock: socket.socket) -> int:
    sock.send(nlmsg(GENL_ID_CTRL, CTRL_CMD_GETFAMILY, nlattr(CTRL_ATTR_FAMILY_NAME, b"TASKSTATS\x00")))
    resp = sock.recv(4096)
    _len, nlmsg_type, _flags, _seq, _port = struct.unpack_from("IHHII", resp)
    if nlmsg_type == NLMSG_ERROR:
        sys.exit(f"GETFAMILY failed — is the taskstats kernel module/config present? {resp!r}")
    attrs = parse_nlattrs(resp[NLMSG_HDRLEN + GENL_HDRLEN:])
    return struct.unpack_from("H", attrs[CTRL_ATTR_FAMILY_ID])[0]


def set_cpumask(sock: socket.socket, family_id: int, attr_type: int, cpumask: str, trailing_nul: bool) -> None:
    payload = cpumask.encode() + (b"\x00" if trailing_nul else b"")
    msg = nlmsg(family_id, TASKSTATS_CMD_GET, nlattr(attr_type, payload), flags=NLM_F_REQUEST | NLM_F_ACK)
    sock.send(msg)
    # This same socket is also our exit-push listener, so a real data message can
    # legitimately be sitting ahead of our own ACK in the receive queue (seen directly:
    # deregistering right after a heavy-delivery phase raced a queued exit-push and
    # grabbed it instead of the ACK) — skip anything that isn't NLMSG_ERROR, bounded so
    # a genuinely missing ACK still fails loudly instead of hanging.
    for _ in range(1000):
        resp = sock.recv(4096)
        _len, nlmsg_type, _flags, _seq, _port = struct.unpack_from("IHHII", resp)
        if nlmsg_type == NLMSG_ERROR:
            break
    else:
        sys.exit(f"never saw an ACK after 1000 messages (cpumask={cpumask!r})")
    (err,) = struct.unpack_from("i", resp, NLMSG_HDRLEN)
    if err != 0:
        sys.exit(f"cpumask op failed: err={err} (cpumask={cpumask!r}, trailing_nul={trailing_nul})")


def generate_pinned_load(label: str, ncpu: int, per_cpu: int) -> None:
    """Spawns per_cpu short-lived processes pinned to EVERY CPU via
    taskset, so exit-push traffic is deliberately spread across all CPUs
    evenly rather than left to whatever the scheduler naturally does."""
    total = per_cpu * ncpu
    issued = 0
    for round_num in range(1, per_cpu + 1):
        procs = [subprocess.Popen(["taskset", "-c", str(cpu), "true"]) for cpu in range(ncpu)]
        for p in procs:
            p.wait()
        issued += len(procs)
        print(f"[{label}] load: round {round_num}/{per_cpu} — {issued}/{total} exits issued",
              file=sys.stderr)


def run_phase(label: str, cpumask: str, trailing_nul: bool, ncpu: int, per_cpu: int, duration: float) -> int:
    sock = socket.socket(socket.AF_NETLINK, socket.SOCK_DGRAM, NETLINK_GENERIC)
    # Bump the receive buffer well above default: the whole point of the workaround
    # phase is that MANY more CPUs start delivering at once, and the default SO_RCVBUF
    # is sized for the (buggy) 4-CPU case — without this, phase 2 can genuinely overflow
    # its own socket (ENOBUFS) purely because the fix is working, which would otherwise
    # crash this demo instead of just reporting a bigger, correct number.
    sock.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 8 * 1024 * 1024)
    sock.bind((0, 0))
    family_id = resolve_family(sock)
    set_cpumask(sock, family_id, TASKSTATS_CMD_ATTR_REGISTER_CPUMASK, cpumask, trailing_nul)
    print(f"[{label}] registered cpumask={cpumask!r} trailing_nul={trailing_nul}", file=sys.stderr)

    sock.settimeout(0.5)
    count = 0
    overflowed = 0
    generate_pinned_load(label, ncpu, per_cpu)

    print(f"[{label}] draining for {duration:.0f}s (this is the window that catches "
          f"exit-push messages for the load just issued)...", file=sys.stderr)
    start = time.monotonic()
    deadline = start + duration  # full drain window starts AFTER load finishes,
    # not before — pinning+waiting on `per_cpu * ncpu` processes itself takes real time,
    # and messages arriving during that time are still caught below, but the window must
    # not be silently eaten by load generation running long
    last_report = start
    while time.monotonic() < deadline:
        try:
            sock.recv(65536)
            count += 1
        except socket.timeout:
            pass
        except OSError as exc:
            if exc.errno != 105:  # ENOBUFS — socket receive buffer overflowed
                raise
            overflowed += 1  # expected, not fatal — see SO_RCVBUF comment above; a real
            # kernel-side receive-buffer overflow drops the message but the socket itself
            # stays usable, so just keep draining rather than treating this as an error
        now = time.monotonic()
        if now - last_report >= 3.0:  # periodic heartbeat — the drain window is otherwise
            # silent for its whole duration, which reads as "hung" rather than "waiting"
            print(f"[{label}] draining: {now - start:.0f}/{duration:.0f}s elapsed, "
                  f"{count} message(s) so far", file=sys.stderr)
            last_report = now

    set_cpumask(sock, family_id, TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK, cpumask, trailing_nul)
    sock.close()
    if overflowed:
        print(f"[{label}] socket receive buffer overflowed {overflowed} time(s) "
              f"(ENOBUFS) — real count is HIGHER than reported below", file=sys.stderr)
    print(f"[{label}] received {count} exit-push messages over {duration}s "
          f"({per_cpu * ncpu} pinned exits issued, 1-per-CPU x {per_cpu} rounds across {ncpu} CPUs)",
          file=sys.stderr)
    return count


def main() -> None:
    if os.geteuid() != 0:
        sys.exit("must run as root (CAP_NET_ADMIN required for REGISTER_CPUMASK)")

    per_cpu = int(sys.argv[1]) if len(sys.argv) > 1 else 10
    duration = float(sys.argv[2]) if len(sys.argv) > 2 else 30.0
    ncpu = os.cpu_count()
    cpumask = f"0-{ncpu - 1}"

    print(f"host: {ncpu} CPUs, cpumask string under test: {cpumask!r} ({len(cpumask)} bytes)", file=sys.stderr)
    if ncpu < 11:
        print(f"NOTE: fewer than 11 CPUs online — {cpumask!r} is only {len(cpumask)} bytes, so the "
              f"same off-by-one bug truncates it to something too short to be a valid range "
              f"(e.g. '0-9' -> '0-') instead of a silently-smaller-but-still-valid one. Expect "
              f"phase 1 to fail outright with EINVAL rather than silently under-deliver — see "
              f"BUG_REPORT.txt's own TEST SYSTEM REQUIREMENT note. The bug is the same either way;"
              f" this script's before/after MESSAGE COUNT comparison just needs >= 11 CPUs to work.",
              file=sys.stderr)
    print(file=sys.stderr)

    buggy = run_phase("phase 1: plain (buggy)", cpumask, trailing_nul=False,
                       ncpu=ncpu, per_cpu=per_cpu, duration=duration)
    print(file=sys.stderr)
    fixed = run_phase("phase 2: +trailing NUL (workaround)", cpumask, trailing_nul=True,
                       ncpu=ncpu, per_cpu=per_cpu, duration=duration)

    print(file=sys.stderr)
    print("=" * 72, file=sys.stderr)
    print(f"plain cpumask={cpumask!r}:            {buggy} messages received", file=sys.stderr)
    print(f"same cpumask + trailing NUL byte: {fixed} messages received", file=sys.stderr)
    if fixed > buggy * 2:
        print(file=sys.stderr)
        print("Reproduced: appending a single NUL byte to the SAME cpumask string", file=sys.stderr)
        print("dramatically changes how many CPUs' exit-push data is delivered.", file=sys.stderr)
        print("This should not happen for a semantically-identical request — see", file=sys.stderr)
        print("MEMO.md in this same directory for the exact root cause in", file=sys.stderr)
        print("kernel/taskstats.c's parse() / lib/nlattr.c's nla_strscpy().", file=sys.stderr)
    else:
        print(file=sys.stderr)
        print("Did NOT clearly reproduce this run — could be a kernel where this is", file=sys.stderr)
        print("already fixed, a very quiet host (low natural process churn), or", file=sys.stderr)
        print("worth a longer --duration / higher exits-per-cpu.", file=sys.stderr)


if __name__ == "__main__":
    main()

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-21 12:52 [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string Oleg Deomi
@ 2026-07-21 23:38 ` Andrew Morton
  2026-07-23 13:04 ` Bradley Morgan
  1 sibling, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2026-07-21 23:38 UTC (permalink / raw)
  To: Oleg Deomi; +Cc: Balbir Singh, linux-kernel

On Tue, 21 Jul 2026 15:52:33 +0300 Oleg Deomi <oleg.deomi@gmail.com> wrote:

> TASKSTATS REGISTER_CPUMASK silently drops the last byte of the cpumask
> string it's given, due to a destination-buffer-sizing bug in
> kernel/taskstats.c's parse(). A request for "0-31" is silently parsed as
> "0-3" instead — the call still succeeds (ACK, no error), so the caller
> has no way to detect that most of the CPUs it asked to cover were never
> actually registered.

Thanks, I queued this fix:


From: Andrew Morton <akpm@linux-foundation.org>
Subject: kernel/taskstats.c:parse(): fix buffer allocation size
Date: Tue Jul 21 04:30:41 PM PDT 2026

parse() is failing to allow for the nla_strscpy()'s null termination of
the cpumask string.  This can result in the cpumask string being
truncated.

This can result in an inappropriate -EINVAL failure or in reporting for an
incorrect span of CPUs.

Fixes: f9fd8914c1ac ("[PATCH] per-task delay accounting taskstats interface: control exit data through cpumasks")
Reported-by: Oleg Deomi <oleg.deomi@gmail.com>
Closes: https://lore.kernel.org/CAByWkfZ6b1=3H9pwkz-dDQOs9cZaF-HYQ6b9Yb0=Hq2r1Vv_Pw@mail.gmail.com
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/taskstats.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/taskstats.c~a
+++ a/kernel/taskstats.c
@@ -368,7 +368,7 @@ static int parse(struct nlattr *na, stru
 		return -E2BIG;
 	if (len < 1)
 		return -EINVAL;
-	data = kmalloc(len, GFP_KERNEL);
+	data = kmalloc(len + 1, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 	nla_strscpy(data, na, len);
_


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-21 12:52 [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string Oleg Deomi
  2026-07-21 23:38 ` Andrew Morton
@ 2026-07-23 13:04 ` Bradley Morgan
  2026-07-23 19:28   ` Andrew Morton
  1 sibling, 1 reply; 8+ messages in thread
From: Bradley Morgan @ 2026-07-23 13:04 UTC (permalink / raw)
  To: oleg.deomi; +Cc: akpm, bsingharora, linux-kernel

Hi Andrew,

I dont think this actually fixes it... :(

[...]

> -	data = kmalloc(len, GFP_KERNEL);
> +	data = kmalloc(len + 1, GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  	nla_strscpy(data, na, len);

nla_strscpy() copies at most dstsize - 1 bytes, and dstsize is still
len here. When the attr payload comes in without a trailing NUL,
srclen == len >= dstsize and the last byte still gets cut off. The
bigger buffer just adds a byte nothing ever writes to (the zero pad
in nla_strscpy() only goes up to dstsize).

Olegs report even spells this out in its suggested fix direction:
allocate len + 1 *and* pass len + 1 as dstsize. The patch only
picked up the first half. The fix is something like:

	data = kmalloc(len + 1, GFP_KERNEL);
	if (!data)
		return -ENOMEM;
	nla_strscpy(data, na, len + 1);

or skip the dance entirely, nla_strdup() already does exactly this:

	data = nla_strdup(na, GFP_KERNEL);
	if (!data)
		return -ENOMEM;

Btw the bug only bites when the sender doesnt NUL terminate the
attr payload; senders that include the NUL (srclen gets decremented
for the trailing NUL, so srclen < dstsize) were always fine. Thats
probably why this survived 20 years. Might be worth a line in
the changelog. And as the report already notes, the policy is
NLA_STRING, not NLA_NUL_STRING, so a payload without the trailing
NUL is legit input here.
Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-23 13:04 ` Bradley Morgan
@ 2026-07-23 19:28   ` Andrew Morton
  2026-07-23 21:11     ` Bradley Morgan
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2026-07-23 19:28 UTC (permalink / raw)
  To: Bradley Morgan; +Cc: oleg.deomi, bsingharora, linux-kernel

On Thu, 23 Jul 2026 14:04:25 +0100 Bradley Morgan <include@grrlz.net> wrote:

> Hi Andrew,
> 
> I dont think this actually fixes it... :(
> 

Thanks.  Obviously I did't try very hard.

> 
> > -	data = kmalloc(len, GFP_KERNEL);
> > +	data = kmalloc(len + 1, GFP_KERNEL);
> >  	if (!data)
> >  		return -ENOMEM;
> >  	nla_strscpy(data, na, len);
> 
> nla_strscpy() copies at most dstsize - 1 bytes, and dstsize is still
> len here. When the attr payload comes in without a trailing NUL,
> srclen == len >= dstsize and the last byte still gets cut off. The
> bigger buffer just adds a byte nothing ever writes to (the zero pad
> in nla_strscpy() only goes up to dstsize).
> 
> Olegs report even spells this out in its suggested fix direction:
> allocate len + 1 *and* pass len + 1 as dstsize. The patch only
> picked up the first half. The fix is something like:
> 
> 	data = kmalloc(len + 1, GFP_KERNEL);
> 	if (!data)
> 		return -ENOMEM;
> 	nla_strscpy(data, na, len + 1);
> 
> or skip the dance entirely, nla_strdup() already does exactly this:
> 
> 	data = nla_strdup(na, GFP_KERNEL);
> 	if (!data)
> 		return -ENOMEM;
> 
> Btw the bug only bites when the sender doesnt NUL terminate the
> attr payload; senders that include the NUL (srclen gets decremented
> for the trailing NUL, so srclen < dstsize) were always fine. Thats
> probably why this survived 20 years. Might be worth a line in
> the changelog. And as the report already notes, the policy is
> NLA_STRING, not NLA_NUL_STRING, so a payload without the trailing
> NUL is legit input here.

OK, I'll drop this.  Please send along a fix sometime.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-23 19:28   ` Andrew Morton
@ 2026-07-23 21:11     ` Bradley Morgan
  2026-07-23 23:17       ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Bradley Morgan @ 2026-07-23 21:11 UTC (permalink / raw)
  To: Andrew Morton; +Cc: oleg.deomi, bsingharora, linux-kernel

On 23 July 2026 20:28:25 BST, Andrew Morton <akpm@linux-foundation.org>
wrote:
>On Thu, 23 Jul 2026 14:04:25 +0100 Bradley Morgan <include@grrlz.net>
>wrote:
>
>> Hi Andrew,
>> 
>> I dont think this actually fixes it... :(
>> 
>
>Thanks.  Obviously I did't try very hard.
>
>> 
>> > -	data = kmalloc(len, GFP_KERNEL);
>> > +	data = kmalloc(len + 1, GFP_KERNEL);
>> >  	if (!data)
>> >  		return -ENOMEM;
>> >  	nla_strscpy(data, na, len);
>> 
>> nla_strscpy() copies at most dstsize - 1 bytes, and dstsize is still
>> len here. When the attr payload comes in without a trailing NUL,
>> srclen == len >= dstsize and the last byte still gets cut off. The
>> bigger buffer just adds a byte nothing ever writes to (the zero pad
>> in nla_strscpy() only goes up to dstsize).
>> 
>> Olegs report even spells this out in its suggested fix direction:
>> allocate len + 1 *and* pass len + 1 as dstsize. The patch only
>> picked up the first half. The fix is something like:
>> 
>> 	data = kmalloc(len + 1, GFP_KERNEL);
>> 	if (!data)
>> 		return -ENOMEM;
>> 	nla_strscpy(data, na, len + 1);
>> 
>> or skip the dance entirely, nla_strdup() already does exactly this:
>> 
>> 	data = nla_strdup(na, GFP_KERNEL);
>> 	if (!data)
>> 		return -ENOMEM;
>> 
>> Btw the bug only bites when the sender doesnt NUL terminate the
>> attr payload; senders that include the NUL (srclen gets decremented
>> for the trailing NUL, so srclen < dstsize) were always fine. Thats
>> probably why this survived 20 years. Might be worth a line in
>> the changelog. And as the report already notes, the policy is
>> NLA_STRING, not NLA_NUL_STRING, so a payload without the trailing
>> NUL is legit input here.
>
>OK, I'll drop this.  Please send along a fix sometime.
>


Hi Andrew, let me be honest, I CBA to do a new thread, here's my suggestion


From 0d5d963d0497e3d2c739e8b5c1403f707054e543 Mon Sep 17 00:00:00 2001
From: Bradley Morgan <include@grrlz.net>
Date: Thu, 23 Jul 2026 21:09:22 +0000
Subject: [PATCH] taskstats: fix cpumask parsing cutting off the last character

parse() hands nla_strscpy() len as dstsize, and nla_strscpy() copies
at most dstsize - 1 bytes. When the attr payload comes in without a
trailing NUL, srclen == len >= dstsize and the last character of the
cpumask string gets cut off. Register "0-15" and you are silently
listening on "0-1", exit data for the rest never shows up.

The bug only bites when the sender doesnt NUL terminate the payload;
senders that include the NUL were always fine (srclen gets decremented
for the trailing NUL, so srclen < dstsize). Thats probably why this
survived 20 years. And the policy is NLA_STRING, not NLA_NUL_STRING,
so a payload without the trailing NUL is legit input here.

Skip the kmalloc/nla_strscpy dance entirely and use nla_strdup(),
which already allocates srclen + 1 and terminates. The nla_len()
bounds checks stay as they were.

Fixes: f9fd8914c1ac ("[PATCH] per-task delay accounting taskstats interface: control exit data through cpumasks")
Reported-by: Oleg Deomi <oleg.deomi@gmail.com>
Closes: https://lore.kernel.org/CAByWkfZ6b1=3H9pwkz-dDQOs9cZaF-HYQ6b9Yb0=Hq2r1Vv_Pw@mail.gmail.com
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/taskstats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index d19bff9..632ca95 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -371,10 +371,9 @@ static int parse(struct nlattr *na, struct cpumask *mask)
 		return -E2BIG;
 	if (len < 1)
 		return -EINVAL;
-	data = kmalloc(len, GFP_KERNEL);
+	data = nla_strdup(na, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-	nla_strscpy(data, na, len);
 	ret = cpulist_parse(data, mask);
 	kfree(data);
 	return ret;
-- 
2.49.0


I better hope my mail client doesn't break the hell out of this! :)

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-23 21:11     ` Bradley Morgan
@ 2026-07-23 23:17       ` Andrew Morton
  2026-07-23 23:19         ` Bradley Morgan
  2026-07-24 15:32         ` Bradley Morgan
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Morton @ 2026-07-23 23:17 UTC (permalink / raw)
  To: Bradley Morgan; +Cc: oleg.deomi, bsingharora, linux-kernel

On Thu, 23 Jul 2026 22:11:43 +0100 Bradley Morgan <include@grrlz.net> wrote:

>...
>
> Hi Andrew, let me be honest, I CBA to do a new thread, here's my suggestion

Seems this fooled Sashiko, so no AI review for you!

> From: Bradley Morgan <include@grrlz.net>
> Date: Thu, 23 Jul 2026 21:09:22 +0000
> Subject: [PATCH] taskstats: fix cpumask parsing cutting off the last character
> 
> parse() hands nla_strscpy() len as dstsize, and nla_strscpy() copies
> at most dstsize - 1 bytes. When the attr payload comes in without a
> trailing NUL, srclen == len >= dstsize and the last character of the
> cpumask string gets cut off. Register "0-15" and you are silently
> listening on "0-1", exit data for the rest never shows up.
> 
> The bug only bites when the sender doesnt NUL terminate the payload;
> senders that include the NUL were always fine (srclen gets decremented
> for the trailing NUL, so srclen < dstsize). Thats probably why this
> survived 20 years. And the policy is NLA_STRING, not NLA_NUL_STRING,
> so a payload without the trailing NUL is legit input here.
> 
> Skip the kmalloc/nla_strscpy dance entirely and use nla_strdup(),
> which already allocates srclen + 1 and terminates. The nla_len()
> bounds checks stay as they were.
> 
> ...
>
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -371,10 +371,9 @@ static int parse(struct nlattr *na, struct cpumask *mask)
>  		return -E2BIG;
>  	if (len < 1)
>  		return -EINVAL;
> -	data = kmalloc(len, GFP_KERNEL);
> +	data = nla_strdup(na, GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> -	nla_strscpy(data, na, len);
>  	ret = cpulist_parse(data, mask);
>  	kfree(data);
>  	return ret;

Nice, thanks,  There's a nla_strdup(), who knew?

(nla_strdup() could use kstrdup(), btw?)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-23 23:17       ` Andrew Morton
@ 2026-07-23 23:19         ` Bradley Morgan
  2026-07-24 15:32         ` Bradley Morgan
  1 sibling, 0 replies; 8+ messages in thread
From: Bradley Morgan @ 2026-07-23 23:19 UTC (permalink / raw)
  To: Andrew Morton; +Cc: oleg.deomi, bsingharora, linux-kernel

On July 24, 2026 12:17:09 AM GMT+01:00, Andrew Morton
<akpm@linux-foundation.org> wrote:
>On Thu, 23 Jul 2026 22:11:43 +0100 Bradley Morgan <include@grrlz.net>
>wrote:
>
>>...
>>
>> Hi Andrew, let me be honest, I CBA to do a new thread, here's my
>suggestion
>
>Seems this fooled Sashiko, so no AI review for you!

if you must do ai review! :)

just put it into Gemini, (preferably on a coding agent, not on the app, or
it'll just say complete bullcrap)



>> From: Bradley Morgan <include@grrlz.net>
>> Date: Thu, 23 Jul 2026 21:09:22 +0000
>> Subject: [PATCH] taskstats: fix cpumask parsing cutting off the last
>character
>> 
>> parse() hands nla_strscpy() len as dstsize, and nla_strscpy() copies
>> at most dstsize - 1 bytes. When the attr payload comes in without a
>> trailing NUL, srclen == len >= dstsize and the last character of the
>> cpumask string gets cut off. Register "0-15" and you are silently
>> listening on "0-1", exit data for the rest never shows up.
>> 
>> The bug only bites when the sender doesnt NUL terminate the payload;
>> senders that include the NUL were always fine (srclen gets decremented
>> for the trailing NUL, so srclen < dstsize). Thats probably why this
>> survived 20 years. And the policy is NLA_STRING, not NLA_NUL_STRING,
>> so a payload without the trailing NUL is legit input here.
>> 
>> Skip the kmalloc/nla_strscpy dance entirely and use nla_strdup(),
>> which already allocates srclen + 1 and terminates. The nla_len()
>> bounds checks stay as they were.
>> 
>> ...
>>
>> --- a/kernel/taskstats.c
>> +++ b/kernel/taskstats.c
>> @@ -371,10 +371,9 @@ static int parse(struct nlattr *na, struct cpumask
>*mask)
>>  		return -E2BIG;
>>  	if (len < 1)
>>  		return -EINVAL;
>> -	data = kmalloc(len, GFP_KERNEL);
>> +	data = nla_strdup(na, GFP_KERNEL);
>>  	if (!data)
>>  		return -ENOMEM;
>> -	nla_strscpy(data, na, len);
>>  	ret = cpulist_parse(data, mask);
>>  	kfree(data);
>>  	return ret;
>
>Nice, thanks,  There's a nla_strdup(), who knew?

Heh.


>(nla_strdup() could use kstrdup(), btw?)

ehhh. does it matter? Heh.



Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string
  2026-07-23 23:17       ` Andrew Morton
  2026-07-23 23:19         ` Bradley Morgan
@ 2026-07-24 15:32         ` Bradley Morgan
  1 sibling, 0 replies; 8+ messages in thread
From: Bradley Morgan @ 2026-07-24 15:32 UTC (permalink / raw)
  To: Andrew Morton; +Cc: oleg.deomi, bsingharora, linux-kernel

On 24 July 2026 00:17:09 BST, Andrew Morton <akpm@linux-foundation.org>
wrote:
>On Thu, 23 Jul 2026 22:11:43 +0100 Bradley Morgan <include@grrlz.net>
>wrote:
>
>>...
>>
>> Hi Andrew, let me be honest, I CBA to do a new thread, here's my
>suggestion
>
>Seems this fooled Sashiko, so no AI review for you!
>
>> From: Bradley Morgan <include@grrlz.net>
>> Date: Thu, 23 Jul 2026 21:09:22 +0000
>> Subject: [PATCH] taskstats: fix cpumask parsing cutting off the last
>character
>> 
>> parse() hands nla_strscpy() len as dstsize, and nla_strscpy() copies
>> at most dstsize - 1 bytes. When the attr payload comes in without a
>> trailing NUL, srclen == len >= dstsize and the last character of the
>> cpumask string gets cut off. Register "0-15" and you are silently
>> listening on "0-1", exit data for the rest never shows up.
>> 
>> The bug only bites when the sender doesnt NUL terminate the payload;
>> senders that include the NUL were always fine (srclen gets decremented
>> for the trailing NUL, so srclen < dstsize). Thats probably why this
>> survived 20 years. And the policy is NLA_STRING, not NLA_NUL_STRING,
>> so a payload without the trailing NUL is legit input here.
>> 
>> Skip the kmalloc/nla_strscpy dance entirely and use nla_strdup(),
>> which already allocates srclen + 1 and terminates. The nla_len()
>> bounds checks stay as they were.
>> 
>> ...
>>
>> --- a/kernel/taskstats.c
>> +++ b/kernel/taskstats.c
>> @@ -371,10 +371,9 @@ static int parse(struct nlattr *na, struct cpumask
>*mask)
>>  		return -E2BIG;
>>  	if (len < 1)
>>  		return -EINVAL;
>> -	data = kmalloc(len, GFP_KERNEL);
>> +	data = nla_strdup(na, GFP_KERNEL);
>>  	if (!data)
>>  		return -ENOMEM;
>> -	nla_strscpy(data, na, len);
>>  	ret = cpulist_parse(data, mask);
>>  	kfree(data);
>>  	return ret;
>
>Nice, thanks,  There's a nla_strdup(), who knew?
>
>(nla_strdup() could use kstrdup(), btw?)
>



Hey, Oleg, is it fixed? apply the changes on your tree and test:d

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-07-24 15:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-21 12:52 [BUG] taskstats: REGISTER_CPUMASK silently truncates the last byte of the cpumask string Oleg Deomi
2026-07-21 23:38 ` Andrew Morton
2026-07-23 13:04 ` Bradley Morgan
2026-07-23 19:28   ` Andrew Morton
2026-07-23 21:11     ` Bradley Morgan
2026-07-23 23:17       ` Andrew Morton
2026-07-23 23:19         ` Bradley Morgan
2026-07-24 15:32         ` Bradley Morgan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®