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

lei lu
2024-07-09T03:16:36Z

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