Re: ovl_copy_xattr() can write uninitialised heap into the upper file's xattr

On Mon, Aug 24, 2026 at 12:44 AM Fahad Alharbi <[email protected]> wrote:
>
> Everything below is now measured on mainline.  My earlier mails in this thread
> carried v6.12.101 numbers and I had not run a mainline kernel; that is fixed,
> and the 6.12 figures are withdrawn in favour of the ones here.
>
> Summary: on v7.2-rc6 an unprivileged user leaks kernel pointers out of
> ovl_copy_xattr(), 5 of 5 boots, and the patch below stops it, 0 of 5.
>
> The bug
> -------
>
> fs/overlayfs/copy_up.c, ovl_copy_xattr(), per xattr name:
>
>         retry:
>                 size = ovl_do_getxattr(oldpath, name, value, value_size);
>                 if (size == -ERANGE)
>                         size = ovl_do_getxattr(oldpath, name, NULL, 0);
>
>                 if (size < 0) { error = size; break; }
>
>                 if (size > value_size) {
>                         ... kvmalloc(size) ...; value_size = size; goto retry;
>                 }
>
>                 error = ovl_do_setxattr(OVL_FS(sb), new, name, value, size, 0);
>
> The second call passes NULL/0, so it reports the size without writing anything
> into `value`.  If the value shrank between the two calls so that the new size
> fits the existing buffer, "size > value_size" is false, the reallocation is
> skipped, and ovl_do_setxattr() is handed `size` bytes of a buffer that was
> never filled for this name.  The allocation is kvmalloc(), not kvzalloc().
>
> Those bytes are persisted as an xattr on the upper file and read back by the
> caller.
>
> Measured on v7.2-rc6 (075b74841bd0)
> -----------------------------------
>
> Unprivileged: uid 1000, CapInh/CapPrm/CapEff all zero (CapBnd is the full set
> inherited from init, which grants nothing without Prm/Eff), exactly one
> unshare(CLONE_NEWUSER|CLONE_NEWNS) since ovl_fs_type has FS_USERNS_MOUNT.  The
> lower filesystem is tmpfs.  No FUSE: /dev/fuse is 0600 on this image, so it
> could not be used even accidentally.  One fresh disposable VM per boot.
>
>                                   boots leaked   pooled            with a kptr
>         unpatched                 5 of 5         122 of 5,000,000  121
>         patched                   0 of 5         0 of 5,000,000    0
>         unpatched, racer disabled 0 of 4         0 of 4,000,000    0
>
> The third row is the control: the leak needs the race, and the detector has no
> false positives.  Cost to an attacker is about 51 s of racing per leaked value
> at ~9,100 copy-ups/s.
>
> A representative line, verbatim:
>
>         [poc] uid=1000 euid=1000 N=32 delta=8 trials=1000000 mode=r
>         [poc] RESULT mode=r N=32 delta=8 : 23 of 1000000 copy-ups leaked
>               (kptr-bearing 23)  [109.5s, 9129/s]
>         [poc] VERDICT: LEAK OBSERVED
>
> What comes out
> --------------
>
> With a slab groom, 32 of 32 trials disclosed at least one kernel pointer, and
> 31 of 32 disclosed the caller's own struct cred address; 129 pointers across
> 3,072 disclosed bytes.  One trial landed on a different object and returned a
> .text pointer, ffffffff8142713e, which defeats KASLR.  SLAB_FREELIST_RANDOM
> and SLAB_FREELIST_HARDENED were both on.  Example:
>
>         [t00] len=96 nz=40 kptrs=4 create@68 cred@40=ffff888102a8d240
>               kptrs: +24=ffff88810535e140 +32=ffff8881019cfdc0
>                      +40=ffff888102a8d240 +48=ffff88810247be70
>
> Without a groom the same leak returns recycled heap rather than pointers -- in
> one 4096-byte sample the overlay's own mount-option string, a path, userspace
> stack addresses and a canary-shaped qword, but no kernel pointer.  I mention
> that so the pointer figures are not read as unconditional: the groom is what
> makes it a pointer leak.
>
> One caveat on those two paragraphs: they were produced by the deterministic
> FUSE probe, which needs /dev/fuse and therefore ran as root.  They establish
> what the kernel discloses.  The unprivileged reachability is the tmpfs table
> above, where 121 of 122 leaks were also kptr-bearing.
>
> Still present in mainline
> -------------------------
>
> ovl_copy_xattr() is byte-identical at origin/master (66fb95a52111,
> 2026-08-22), v7.2-rc6, v6.12.101 and v6.12.104 -- same sha256 of the
> extracted function.
> git log -L :ovl_copy_xattr:fs/overlayfs/copy_up.c returns 16 commits ever, the
> newest a 2024 LSM return-value refactor; no commit has ever added zeroing, a
> bound, or a re-read.  Introduced by e4ad29fa0d22 ("ovl: use a minimal buffer
> in ovl_copy_xattr", v4.5), confirmed by content since v4.4 has no value_size.
> Nothing overlayfs-related is in the stable queues.
>
> The patch
> ---------
>
> Built and booted on v7.2-rc6.  Both kernels use byte-identical configs, so the
> only variable is this hunk; I confirmed the change is in the binary
> (objdump shows cmp $0x4 and mov $0xfffffff5 in the patched copy_up.o, absent
> from the unpatched one) rather than trusting the build.
>
>   - Leak: 0 of 5,000,000 copy-ups, 0 of 5 boots, against 122 of 5,000,000
>     unpatched.
>   - Functional: 51 of 51 xattr byte-compares identical to the unpatched
>     kernel, 17 sizes from 0 to 65536 in three orderings, with the
>     legitimate -ERANGE growth path exercised and no -EAGAIN.
>   - Termination: the probe that failed to terminate on three earlier attempts
>     of mine now returns in 0.100 s after 5 sized GETXATTRs, exactly
>     1 + OVL_COPY_XATTR_MAX_RETRY.
>   - -EAGAIN was never reached in normal operation: EAGAIN=0 on all 10
>     unprivileged boots.
>   - No BUG, WARNING, oops, lockup or stall in any boot.
>
> Four things about the patch you should have rather than find
> ------------------------------------------------------------
>
> 1. It does not fix the growth path.  The "size > value_size" branch still does
>    goto retry with no counter.  It is bounded -- value_size increases
>    strictly, and fuse_getxattr() clamps a size query with
>    min_t(size_t, outarg.size, XATTR_SIZE_MAX) (fs/fuse/xattr.c) -- so it
>    cannot loop forever, but a hostile lower fs can still force up to ~65,535
>    kvmalloc/kvfree plus getxattr round-trips for a single name.  Measured with
>    an ever-increasing size: unpatched 65,536 calls / 7.815 s, patched 65,540 /
>    7.810 s.  Pre-existing and unchanged by this patch; my patch costs four
>    extra iterations there.  I raise it because it is reachable today.
>
> 2. I suggested kvzalloc() in my first mail and did not take that route.
>    kvzalloc() would stop the disclosure in one word, and if you want the
>    minimal fix that is the one to take.  I went further because the shrink
>    case is also a correctness bug independent of the leak: the upper file
>    still ends up with bytes that are not the lower file's xattr value --
>    zeros instead of heap.  Re-reading writes the right value.  If you would
>    rather have the one-word change, or want both (kvzalloc as a belt-and-
>    braces plus the re-read), say which and I will send it.
>
> 3. -EAGAIN is a new errno for overlayfs; git grep EAGAIN fs/overlayfs was
>    empty before this.  No kernel caller of copy-up retries on it, but
>    userspace conventionally does, and overlayfs already uses -EIO a few lines above for
>    "the underlying fs is broken".  If you would prefer -EIO, say so and I will
>    resend.
>
> 4. This is my fourth attempt.  Three earlier versions failed to terminate; one
>    had no progress measure and two grew value_size on every -ERANGE on the
>    assumption that XATTR_SIZE_MAX bounded it, which is wrong for this path
>    because ovl_do_getxattr() -> vfs_getxattr() passes the caller's buffer size
>    to __vfs_getxattr() unbounded.  I did not send any of those, and this
>    version counts rather than relying on an implicit bound.
>
> Reproducers and logs
> --------------------
>
> I have three reproducers -- the unprivileged tmpfs racer, a deterministic FUSE
> probe with eight attack modes, and a functional-regression harness -- plus the
> unedited console logs for every run quoted above.  Per security-bugs.rst I am
> not pasting the reproducer here, since this thread will be posted publicly;
> say the word and I will send any or all of it, in whatever form you prefer.
>
> I have not Cced linux-unionfs or LKML, since this is still on
> [email protected].  If you would rather handle it in the open, say so and I
> will repost there.

Fahad,

I usually don't CC public lists for private emails, but since you offered it,
I will reply on the list.

First some ethics w.r.t LLM driver patches:
(1) The Linux kernel generally does not consider LLM detected security issues
for security embargoes.
I won't fault you if your conscience tells you that you need to post privately
first for a very severe security bug, but other maintainers may be less tolerant
so keep that in mind.
(2) You are writing to HUMANS not to machines. This report is way too long
and a human would spend way too long before realizing this is worth their
effort or not. Please use some of your token to produce a clear human readable
TDLR the next time you post LLM driven patches and honestly, I am sure that
the report itself could have been much shorter if you read it and tried to
make it fit a human reader who is a volunteer with limited time.
(3) Please make sure that LLM reads overlayfs.rst before proposing patches

Regarding the patch itself, if you follow rule #3 above you will find
this paragraph:

Changes to underlying filesystems
---------------------------------

Changes to the underlying filesystems while part of a mounted overlay
filesystem are not allowed.  If the underlying filesystem is changed,
the behavior of the overlay is undefined, though it will not result in
a crash or deadlock.

What this tells you is that your EAGAIN retry loop is the wrong approach -
We do not try to cope with changes to underlying layers - we detect them
and abort with EIO, something like that should be enough:

    size = ovl_do_getxattr(oldpath, name, value, value_size);
    if (size == -ERANGE) {
        size = ovl_do_getxattr(oldpath, name, NULL, 0);
        if (size >= 0 && size <= value_size) {
            error = -EIO;
            break;
        }
    }

The next time you post please do it on the list and please make it concise.

Thanks,
Amir.

>
> -- 8< --------------------------------------------------------------- 8< --
>
> From: Fahad Alharbi <[email protected]>
> Subject: [PATCH] ovl: don't copy an xattr the buffer was never filled with
>
> ovl_copy_xattr() sizes and fills one reusable buffer per xattr name:
>
>         size = ovl_do_getxattr(oldpath, name, value, value_size);
>         if (size == -ERANGE)
>                 size = ovl_do_getxattr(oldpath, name, NULL, 0);
>
> The second call passes NULL/0, so it reports the current size without
> writing anything into `value`.  When the value grew (giving -ERANGE) and
> then shrank again before the sizing call, the reported size can be less
> than or equal to value_size.  The "size > value_size" reallocation path
> is then skipped and the code falls through to
>
>         error = ovl_do_setxattr(OVL_FS(sb), new, name, value, size, 0);
>
> writing `size` bytes out of a buffer that was never filled for this name.
>
> For the first name that is uninitialised kvmalloc() memory (the buffer is
> kvmalloc(), not kvzalloc()); for later names it is the value of a
> previously copied xattr of the same file, which the caller may not have
> permission to read.  Either way the bytes are persisted as an xattr on
> the upper file and can be read back by an unprivileged user.  Measured on
> an unmodified 6.12.101 as uid 1000 with no capabilities and one user
> namespace, with a tmpfs lower and a second thread changing the value
> size: 624 of 2,000,000 copy-ups leaked over 20 cold boots, and 397 of
> those carried a kernel pointer.  A hostile FUSE lower makes it
> deterministic and much larger: one chmod() over 4096 xattr names returned
> 819,200 bytes of kernel memory on that same kernel.
>
> Re-read the value when the buffer was not filled for this name.  A lower
> filesystem that keeps changing the size could make that repeat, so bound
> the number of re-reads: only a racing or hostile lower fs reaches the
> retry at all, and a well-behaved one needs none.  The counter is per
> name, so a well-behaved name never spends another name's budget, and the
> abort happens before ovl_do_setxattr(), so the failure mode is a failed
> copy-up and never a partial or leaking one.
>
> Note this does not fix the pre-existing "size > value_size" growth path,
> which still does an uncounted goto retry; against a lower that reports an
> ever-increasing size that path runs to XATTR_SIZE_MAX iterations today,
> bounded only by the lower filesystem (fuse_getxattr() clamps a size query
> to XATTR_SIZE_MAX), not by ovl_copy_xattr().  This patch adds four
> iterations to that worst case and does not otherwise change it.
>
> Testing was done on v6.12.101, both a KASAN + SLUB_DEBUG_ON build and an
> uninstrumented one, in disposable VMs; I have not run a mainline kernel.
> ovl_copy_xattr() is byte-identical between v6.12.101 and v7.2-rc6, and
> the patched function is byte-identical to the one that was built and
> tested.
>
> Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")
> Cc: [email protected]
> Assisted-by: Claude-Code:claude-opus-5
> Signed-off-by: Fahad Alharbi <[email protected]>
> ---
>  fs/overlayfs/copy_up.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index e963701b4c87..6a78dfbf6eb7 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -72,12 +72,20 @@ static int ovl_copy_acl(struct ovl_fs *ofs, const struct path *path,
>         return err;
>  }
>
> +/*
> + * Bound on re-reads of a single xattr whose size changed under us.  Only a
> + * racing or hostile lower filesystem can reach it; a well-behaved one needs
> + * none.
> + */
> +#define OVL_COPY_XATTR_MAX_RETRY 4
> +
>  int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct dentry *new)
>  {
>         struct dentry *old = oldpath->dentry;
>         ssize_t list_size, size, value_size = 0;
>         char *buf, *name, *value = NULL;
>         int error = 0;
> +       int retries;
>         size_t slen;
>
>         if (!old->d_inode->i_op->listxattr || !new->d_inode->i_op->listxattr)
> @@ -129,10 +137,32 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>                         break;
>                 }
>
> +               retries = 0;
>  retry:
>                 size = ovl_do_getxattr(oldpath, name, value, value_size);
> -               if (size == -ERANGE)
> +               if (size == -ERANGE) {
>                         size = ovl_do_getxattr(oldpath, name, NULL, 0);
> +                       if (size >= 0 && size <= value_size) {
> +                               /*
> +                                * The sizing call above reports the length but
> +                                * does not fill the buffer.  Reaching here means
> +                                * the value shrank after the -ERANGE, so it now
> +                                * fits and the reallocation below is skipped --
> +                                * which would hand ovl_do_setxattr() bytes that
> +                                * were never written for this name.  Re-read it.
> +                                *
> +                                * A lower filesystem that keeps changing the
> +                                * size could make that repeat forever, so bound
> +                                * it: this only triggers on a racing or hostile
> +                                * lower fs, never in the steady state.
> +                                */
> +                               if (++retries > OVL_COPY_XATTR_MAX_RETRY) {
> +                                       error = -EAGAIN;
> +                                       break;
> +                               }
> +                               goto retry;
> +                       }
> +               }
>
>                 if (size < 0) {
>                         error = size;
> --
> 2.43.0
>