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

Kees Cook
2024-07-08T19:52:13Z

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