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

[Kees Cook <kees@xxxxxxxxxx>]

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?

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