Re: [PATCH] fs/ntfs3: bound NTFS_DE view.data_off in UpdateRecordData{Root,Allocation}

[Pavitra Jha]

Thanks for fixing this. Since you cited my original report as the
basis, would you mind adding:

Reported-by: Pavitra Jha <jhapavitra98@xxxxxxxxx>

to the patch tags?

On Fri, 15 May 2026 at 22:04, Michael Bommarito
<michael.bommarito@xxxxxxxxx> wrote:
>
> In do_action()'s UpdateRecordDataRoot (fslog.c:3489) and
> UpdateRecordDataAllocation (fslog.c:3697) cases, the memmove
> destination is `Add2Ptr(e, le16_to_cpu(e->view.data_off))`,
> where e->view.data_off comes from an on-disk NTFS_DE inside
> an INDEX_ROOT or INDEX_BUFFER.  Neither case validates
> view.data_off + dlen against e->size; the existing
> check_if_index_root / check_if_alloc_index helpers walk the
> entry chain and validate the entry's offset, but not its
> internal view fields.
>
> The neighbouring read sites (e.g., fs/ntfs3/index.c when
> iterating view entries) check view.data_off + view.data_size
> <= e->size.  Apply the same bound at the two memmove sites.
>
> Reproduced under UML+KASAN on mainline 8d90b09e6741 via
> pr_warn-only probe instrumentation: with view.data_off forced
> to 0xFFFC, the memmove writes 32 bytes past the end of the
> NTFS_DE.
>
> This is similar in shape to Pavitra Jha's 2026-05-02 patch
> "fs/ntfs3: prevent oob in case UpdateRecordDataRoot"
> (<20260502105008.21827-1-jhapavitra98@xxxxxxxxx>) which
> proposes calling ntfs3_bad_de_range(); that helper does not
> exist in mainline.  This patch uses inline checks.
>
> Fixes: b46acd6a6a62 ("fs/ntfs3: Add NTFS journal")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
> ---
>  fs/ntfs3/fslog.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
> index acfa18b84401e..127860fd2ab50 100644
> --- a/fs/ntfs3/fslog.c
> +++ b/fs/ntfs3/fslog.c
> @@ -3497,6 +3497,18 @@ static int do_action(struct ntfs_log *log, struct OPEN_ATTR_ENRTY *oe,
>
>                 e = Add2Ptr(attr, le16_to_cpu(lrh->attr_off));
>
> +               /*
> +                * e->view.data_off and dlen come from the on-disk
> +                * INDEX_ROOT entry / LRH.  The neighbouring read sites
> +                * (e.g. fs/ntfs3/index.c) check that
> +                * view.data_off + view.data_size <= e->size; mirror that
> +                * bound here so the memmove cannot reach past the entry.
> +                */
> +               if (le16_to_cpu(e->view.data_off) > le16_to_cpu(e->size) ||
> +                   le16_to_cpu(e->view.data_off) + dlen >
> +                           le16_to_cpu(e->size))
> +                       goto dirty_vol;
> +
>                 memmove(Add2Ptr(e, le16_to_cpu(e->view.data_off)), data, dlen);
>
>                 mi->dirty = true;
> @@ -3689,6 +3701,12 @@ static int do_action(struct ntfs_log *log, struct OPEN_ATTR_ENRTY *oe,
>                         goto dirty_vol;
>                 }
>
> +               /* See UpdateRecordDataRoot for the rationale. */
> +               if (le16_to_cpu(e->view.data_off) > le16_to_cpu(e->size) ||
> +                   le16_to_cpu(e->view.data_off) + dlen >
> +                           le16_to_cpu(e->size))
> +                       goto dirty_vol;
> +
>                 memmove(Add2Ptr(e, le16_to_cpu(e->view.data_off)), data, dlen);
>
>                 a_dirty = true;
> --
> 2.53.0
>