[PATCH] [RFC] ntfs: disable for 64KB because of stack overflow risk

[PATCH] [RFC] ntfs: disable for 64KB because of stack overflow risk

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

On ARM64 randconfig builds, we occasionally get warnings for NTFS:

fs/ntfs/aops.c: In function 'ntfs_write_mst_block':
fs/ntfs/aops.c:1328:1: error: the frame size of 2224 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]

The problem here is that with 64KB pages, we get two arrays on the
stack that each have 128 pointers, for a total of 2KB. Before
the VLA change, this could already happen with 512-byte blocks,
however in practice NTFS should usually have 4KB blocks and not
be affected by this (see link).

Now the stack usage is always > 2KB on any architecture with 64KB
pages. Since both NTFS and 64KB page support are fairly rare,
we may get away with just marking the combination as disallowed
in Kconfig and see if anyone complains before we find a different
way to address it.

Fixes: ac4ecf968acb ("ntfs: aops: remove VLA usage")
Link: https://support.microsoft.com/en-us/help/140365/default-cluster-size-for-ntfs-fat-and-exfat
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/ntfs/Kconfig | 1 +
 fs/ntfs/aops.c  | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/fs/ntfs/Kconfig b/fs/ntfs/Kconfig
index 1667a7e590d8..b770f0209b9c 100644
--- a/fs/ntfs/Kconfig
+++ b/fs/ntfs/Kconfig
@@ -2,6 +2,7 @@
 config NTFS_FS
 	tristate "NTFS file system support"
 	select NLS
+	depends on !PAGE_SIZE_64KB && !ARM64_64K_PAGES
 	help
 	  NTFS is the file system of Microsoft Windows NT, 2000, XP and 2003.
 
diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
index bb0a43860ad2..76d59bd4c1eb 100644
--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -914,6 +914,9 @@ static int ntfs_write_mst_block(struct page *page,
 	bool sync, is_mft, page_is_dirty, rec_is_dirty;
 	unsigned char bh_size_bits;
 
+	/* Two arrays of MAX_BUF_PER_PAGE on the stack risks an overrun with 64K pages */
+	BUILD_BUG_ON(PAGE_SIZE >= 65536);
+
 	if (WARN_ON(rec_size < NTFS_BLOCK_SIZE))
 		return -EINVAL;
 
-- 
2.29.2

Re: [PATCH] [RFC] ntfs: disable for 64KB because of stack overflow risk

[Kees Cook]

On Mon, Sep 27, 2021 at 04:18:03PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> On ARM64 randconfig builds, we occasionally get warnings for NTFS:
> 
> fs/ntfs/aops.c: In function 'ntfs_write_mst_block':
> fs/ntfs/aops.c:1328:1: error: the frame size of 2224 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> 
> The problem here is that with 64KB pages, we get two arrays on the
> stack that each have 128 pointers, for a total of 2KB. Before
> the VLA change, this could already happen with 512-byte blocks,
> however in practice NTFS should usually have 4KB blocks and not
> be affected by this (see link).
> 
> Now the stack usage is always > 2KB on any architecture with 64KB
> pages. Since both NTFS and 64KB page support are fairly rare,
> we may get away with just marking the combination as disallowed
> in Kconfig and see if anyone complains before we find a different
> way to address it.
> 
> Fixes: ac4ecf968acb ("ntfs: aops: remove VLA usage")
> Link: https://support.microsoft.com/en-us/help/140365/default-cluster-size-for-ntfs-fat-and-exfat
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

That seems reasonable.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] [RFC] ntfs: disable for 64KB because of stack overflow risk

[Anton Altaparmakov]

Hi Arnd,

Thanks for the patch but what is the problem with the stack usage exceeding 2048 bytes?

I am not aware of any architectures that implements kernel stack size (THREAD_SIZE) less than page size and by default most architectures with 4kiB page size even use two pages to make the stack 8kiB.

Thus on a 64kiB page size system the stack size is minimum 64kiB so using just over 2kiB seems to totally fine to me so why apply your patch?

Seems to me it would be better to fix the warning message you are seeing instead - it is a really stupid warning on a system with 64kiB stack size!

Best regards,

	Anton

> On 27 Sep 2021, at 15:18, Arnd Bergmann <arnd@kernel.org> wrote:
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> On ARM64 randconfig builds, we occasionally get warnings for NTFS:
> 
> fs/ntfs/aops.c: In function 'ntfs_write_mst_block':
> fs/ntfs/aops.c:1328:1: error: the frame size of 2224 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> 
> The problem here is that with 64KB pages, we get two arrays on the
> stack that each have 128 pointers, for a total of 2KB. Before
> the VLA change, this could already happen with 512-byte blocks,
> however in practice NTFS should usually have 4KB blocks and not
> be affected by this (see link).
> 
> Now the stack usage is always > 2KB on any architecture with 64KB
> pages. Since both NTFS and 64KB page support are fairly rare,
> we may get away with just marking the combination as disallowed
> in Kconfig and see if anyone complains before we find a different
> way to address it.
> 
> Fixes: ac4ecf968acb ("ntfs: aops: remove VLA usage")
> Link: https://support.microsoft.com/en-us/help/140365/default-cluster-size-for-ntfs-fat-and-exfat
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> fs/ntfs/Kconfig | 1 +
> fs/ntfs/aops.c  | 3 +++
> 2 files changed, 4 insertions(+)
> 
> diff --git a/fs/ntfs/Kconfig b/fs/ntfs/Kconfig
> index 1667a7e590d8..b770f0209b9c 100644
> --- a/fs/ntfs/Kconfig
> +++ b/fs/ntfs/Kconfig
> @@ -2,6 +2,7 @@
> config NTFS_FS
> 	tristate "NTFS file system support"
> 	select NLS
> +	depends on !PAGE_SIZE_64KB && !ARM64_64K_PAGES
> 	help
> 	  NTFS is the file system of Microsoft Windows NT, 2000, XP and 2003.
> 
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index bb0a43860ad2..76d59bd4c1eb 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -914,6 +914,9 @@ static int ntfs_write_mst_block(struct page *page,
> 	bool sync, is_mft, page_is_dirty, rec_is_dirty;
> 	unsigned char bh_size_bits;
> 
> +	/* Two arrays of MAX_BUF_PER_PAGE on the stack risks an overrun with 64K pages */
> +	BUILD_BUG_ON(PAGE_SIZE >= 65536);
> +
> 	if (WARN_ON(rec_size < NTFS_BLOCK_SIZE))
> 		return -EINVAL;
> 
> -- 
> 2.29.2
> 

Re: [PATCH] [RFC] ntfs: disable for 64KB because of stack overflow risk

[Arnd Bergmann]

On Tue, Sep 28, 2021 at 1:21 AM Anton Altaparmakov <anton@tuxera.com> wrote:
>
> Hi Arnd,
>
> Thanks for the patch but what is the problem with the stack usage exceeding 2048 bytes?
>
> I am not aware of any architectures that implements kernel stack size (THREAD_SIZE)
> less than page size and by default most architectures with 4kiB page size even use two
> pages to make the stack 8kiB.

The two are decoupled these days unless CONFIG_VMAP_STACK is enabled at build
time, in which case the THREAD_SIZE is always a multiple of STACK_SIZE.
No architecture currently forces the use of VMAP_STACK though, so the allocation
is done in alloc_thread_stack_node() using this kmem_cache:

        thread_stack_cache = kmem_cache_create_usercopy("thread_stack",
                                        THREAD_SIZE, THREAD_SIZE, 0, 0,
                                        THREAD_SIZE, NULL);

64K pages are allowed on arm64, powerpc, mips, microblaze, ia64, sh, hexagon
and the upcoming loongarch port. The respective THREAD_SHIFT/THREAD_SIZE
values on these are

arch/arm64/include/asm/memory.h:#define MIN_THREAD_SHIFT (14 +
KASAN_THREAD_SHIFT)
arch/powerpc/Kconfig:config THREAD_SHIFT
arch/powerpc/Kconfig-   default "14" if PPC64
arch/mips/include/asm/thread_info.h:#define THREAD_SIZE_ORDER (0)
arch/mips/include/asm/thread_info.h:#define THREAD_SIZE (PAGE_SIZE <<
THREAD_SIZE_ORDER)
arch/microblaze/include/asm/thread_info.h:#define THREAD_SHIFT 13
arch/ia64/include/asm/ptrace.h:# define KERNEL_STACK_SIZE_ORDER         0
arch/ia64/include/asm/ptrace.h:#define IA64_STK_OFFSET
 ((1 << KERNEL_STACK_SIZE_ORDER)*PAGE_SIZE)
arch/ia64/include/asm/ptrace.h:#define KERNEL_STACK_SIZE
 IA64_STK_OFFSET
arch/ia64/include/asm/thread_info.h:#define THREAD_SIZE
 KERNEL_STACK_SIZE
arch/sh/include/asm/thread_info.h:#define THREAD_SHIFT  12
arch/hexagon/include/asm/thread_info.h:#define THREAD_SHIFT 12

As far as I can tell, only mips and ia64 require the kernel stack to
be a multiple
of the page size here, and I would consider that a bug: This is extremely
wasteful, especially considering that those machines typically won't have the
vast amounts of RAM that modern arm64 and powerpc64 servers have.

On a hexagon or sh system with 4KB stacks, using over 2KB in one function
is definitely excessive. Those machines wouldn't normally need NTFS support,
but that was kind-of the point of my patch.

         Arnd