[PATCH] ntfs3: Add bounds checking for dp0

[PATCH] ntfs3: Add bounds checking for dp0

[lei lu]

Added out-of-bound checking for *dp0 (DIR_PAGE_ENTRY_32).

Signed-off-by: lei lu <llfamsec@gmail.com>
---
 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);
+		if (PtrOffset(dptbl, &dp0->vcn_low) + len > t32) {
+			err = -EINVAL;
+			goto out;
+		}
+		memmove(&dp->vcn, &dp0->vcn_low, len);
 	}
 
 end_conv_1:
-- 
2.34.1

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

[Kees Cook]

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@gmail.com>
> ---
>  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?

> +		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

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

[lei lu]

On Tue, Jul 9, 2024 at 3:52 AM Kees Cook <kees@kernel.org> 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@gmail.com>
> > ---
> >  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