Re: [PATCH] ntfs3: Add bounds checking for dp0

[lei lu <llfamsec@xxxxxxxxx>]

On Tue, Jul 9, 2024 at 3:52 AM Kees Cook <kees@xxxxxxxxxx> wrote:
>
> On Mon, Jul 01, 2024 at 06:29:35PM +0800, lei lu wrote:
> > Added out-of-bound checking for *dp0 (DIR_PAGE_ENTRY_32).
> >
> > Signed-off-by: lei lu <llfamsec@xxxxxxxxx>
> > ---
> >  fs/ntfs3/fslog.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
> > index 855519713bf7..af6f2ce9ea68 100644
> > --- a/fs/ntfs3/fslog.c
> > +++ b/fs/ntfs3/fslog.c
> > @@ -4184,10 +4184,14 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
> >       dp = NULL;
> >       while ((dp = enum_rstbl(dptbl, dp))) {
> >               struct DIR_PAGE_ENTRY_32 *dp0 = (struct DIR_PAGE_ENTRY_32 *)dp;
> > -             // NOTE: Danger. Check for of boundary.
> > -             memmove(&dp->vcn, &dp0->vcn_low,
> > -                     2 * sizeof(u64) +
> > -                             le32_to_cpu(dp->lcns_follow) * sizeof(u64));
> > +             // Check for of boundary.
> > +             u32 len = 2 * sizeof(u64) +
> > +                     le32_to_cpu(dp->lcns_follow) * sizeof(u64);
>
> Can't this calculation still overflow?

Thanks for your time.

I'm a bit confused about this. Do you mean that the value of
lcns_follow is too large？

As you said below, I will split up the memcpy into two steps:
        1) fixed members:
            dp->vcn = ((u64)dp0->vcn_high << 32U | dp0->vcn_low);
            dp->oldest_lsn = ((u64)dp0->oldest_lsn_high << 32U |
dp0->oldest_lsn_low);
        2) variable members:
            memmove(dp->page_lcns, dp0->page_lcns_low, lcns_size);

So the check will be changed to the following code:
u32 lcns_size = le32_to_cpu(dp->lcns_follow) * sizeof(u64);
if (PtrOffset(dptbl, &dp0->page_lcns_low) + lcns_size > t32) {
        err = -EINVAL;
        goto out;
}

The check makes sure that variable members don't stray beyond valid
memory region.
And the enum_rstbl is responsible for checking the boundary of fixed
members. (Refer
to another patch: Add bounds checking to enum_rstbl().)

Thanks,
LL

>
> > +             if (PtrOffset(dptbl, &dp0->vcn_low) + len > t32) {
> > +                     err = -EINVAL;
> > +                     goto out;
> > +             }
> > +             memmove(&dp->vcn, &dp0->vcn_low, len);
> >       }
>
> Hmm, I think this code has not been exercised under CONFIG_FORTIFY_SOURCE.
> This would immediately WARN at run-time: dp->vcn is a u64. This is
> writing beyond the member. Likely this needs to be split up:
>
> struct DIR_PAGE_ENTRY {
>         __le32 next;         // 0x00: RESTART_ENTRY_ALLOCATED if allocated
>         __le32 target_attr;  // 0x04: Index into the Open attribute Table
>         __le32 transfer_len; // 0x08:
>         __le32 lcns_follow;  // 0x0C:
>         __le64 vcn;          // 0x10: Vcn of dirty page
>         __le64 oldest_lsn;   // 0x18:
>         __le64 page_lcns[];  // 0x20:
> };
>
> struct DIR_PAGE_ENTRY_32 {
>         __le32 next;            // 0x00: RESTART_ENTRY_ALLOCATED if allocated
>         __le32 target_attr;     // 0x04: Index into the Open attribute Table
>         __le32 transfer_len;    // 0x08:
>         __le32 lcns_follow;     // 0x0C:
>         __le32 reserved;        // 0x10:
>         __le32 vcn_low;         // 0x14: Vcn of dirty page
>         __le32 vcn_hi;          // 0x18: Vcn of dirty page
>         __le32 oldest_lsn_low;  // 0x1C:
>         __le32 oldest_lsn_hi;   // 0x1C:
>         __le32 page_lcns_low;   // 0x24:
>         __le32 page_lcns_hi;    // 0x24:
> };
>
> dp->vcn = ((u64)dp0->vcn_high << 32U | dp0->vcn_low);
> dp->oldest_lsn = ((u64)dp0->oldest_lsn_high << 32U | dp0->oldest_lsn_low);
>
> memmove(dp->page_lcns, dp0->...?, ...)
>
>
> --
> Kees Cook