Re: [PATCH] fs/ntfs3: Fix heap overflow after ALIGN in mi_pack_runs()

[Konstantin Komarov <almaz.alexandrovich@paragon-software.com>]

On 7/8/26 16:56, Alexandro Calò wrote:

>  From 70cdcdc3203f73fe2a77b70aa32d31140290cc99 Mon Sep 17 00:00:00 2001
> From: Alexandro Calo <alexandro.calo@nozominetworks.com>
> Date: Wed, 8 Jul 2026 16:16:39 +0200
> Subject: [PATCH] fs/ntfs3: Fix heap overflow after ALIGN in mi_pack_runs()
>
> When run_pack() returns a non-aligned byte count, ALIGN(err, 8)
> inflates it by 1-7 bytes, and since the destination of the memmove() is
> computed from that inflated value, the entire attribute tail could be
> copied 1-7 bytes past where the record buffer ends.
>
> The memmove() destination is next + new_run_size - run_size. For it to
> stay within the record, new_run_size must not exceed run_size + dsize,
> the space actually available.
>
> Since new_run_size = ALIGN(err, 8), a fix could be to ensures there is
> enough room that whatever ALIGN does to err, the result still fits within
> the available space. Restricting the budget of run_pack() should
> guarantee that.
>
> Something like:
>      u32 avail = (run_size + dsize) & ~7u;
>      err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail, &plen);
>
> Now, run_pack() should never return 0. As now, the function returns at
> least 1. So this is fine. Negative values are caught as errors by
> if(err<0).
>
> The rounded-down avail may be smaller than the existing run size,
> if avail=0 run_pack is called and will write run_buf[0] = 0; that will be
> OOB, because the buffer size is zero, so imo the caller should avoid
> calling run_pack() with avail=0.
>
> After run_pack(), if plen = 0, then svcn + plen - 1 becomes svcn - 1.
> So, after run_pack(), it may be worth ensuring if(!plen).
>
> Lastly, I'm not sure if -ENOSPC is the right error code here, but
> it seems reasonable for a space condition.
>
> This heap out-of-bounds write requires a crafted filesystem image,
> which is not in the kernel threat model, but fixing memory errors would be
> nice to keep things secure.
>
> Signed-off-by: Alexandro Calo <alexandro.calo@nozominetworks.com>
> ---
>   fs/ntfs3/record.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
> index 32bdb034c2a3..2466dba15241 100644
> --- a/fs/ntfs3/record.c
> +++ b/fs/ntfs3/record.c
> @@ -637,18 +637,32 @@ int mi_pack_runs(struct mft_inode *mi, struct ATTRIB *attr,
>          u32 run_size = asize - run_off;
>          u32 tail = used - aoff - asize;
>          u32 dsize = sbi->record_size - used;
> +       u32 avail;
>
>          /* Make a maximum gap in current record. */
>          memmove(next + dsize, next, tail);
>
> +       /* Leave room for the 8-byte ALIGN() to avoid OOB write */
> +       avail = (run_size + dsize) & ~7u;
> +
> +       if (!avail) {
> +               memmove(next, next + dsize, tail);
> +               return -ENOSPC;
> +       }
> +
>          /* Pack as much as possible. */
> -       err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), run_size + dsize,
> +       err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail,
>                         &plen);
>          if (err < 0) {
>                  memmove(next, next + dsize, tail);
>                  return err;
>          }
>
> +       if (!plen) {
> +               memmove(next, next + dsize, tail);
> +               return -ENOSPC;
> +       }
> +
>          new_run_size = ALIGN(err, 8);
>
>          memmove(next + new_run_size - run_size, next + dsize, tail);
>
> base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53
> --
> 2.47.3

Hello,

Sorry for the delay.
Your patch is applied, thanks.

Regards,
Konstantin