* [PATCH 0/3] NTFS: Remove VLA usage
@ 2018-06-26 17:29 Kees Cook
2018-06-26 17:29 ` [PATCH 1/3] NTFS: aops: " Kees Cook
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kees Cook @ 2018-06-26 17:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Anton Altaparmakov, Al Viro, Jan Kara, linux-ntfs-dev,
linux-kernel
In the quest to remove all stack VLA usage from the kernel[1], this series
removes all the VLAs in use in fs/ntfs/. A couple strategies are used,
and are detailed in the individual patches.
As far as I can tell, -mm was the last place to take NTFS-specific
patches, so I'm aiming this series at Andrew and hoping Anton can
review/ack/etc. :)
Thanks!
-Kees
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Kees Cook (3):
NTFS: aops: Remove VLA usage
NTFS: decompress: Remove VLA usage
NTFS: mft: Remove VLA usage
fs/ntfs/aops.c | 5 ++++-
fs/ntfs/compress.c | 28 ++++++++++++++++------------
fs/ntfs/mft.c | 12 ++++++++++--
3 files changed, 30 insertions(+), 15 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] NTFS: aops: Remove VLA usage
2018-06-26 17:29 [PATCH 0/3] NTFS: Remove VLA usage Kees Cook
@ 2018-06-26 17:29 ` Kees Cook
2018-07-05 20:44 ` Arnd Bergmann
2018-06-26 17:29 ` [PATCH 2/3] NTFS: decompress: " Kees Cook
2018-06-26 17:29 ` [PATCH 3/3] NTFS: mft: " Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-06-26 17:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Anton Altaparmakov, linux-ntfs-dev, Al Viro, Jan Kara,
linux-kernel
In the quest to remove all stack VLA usage from the kernel[1], this
uses the maximum size needed on the stack and adds a sanity check for
robustness: index.block_size cannot be larger than PAGE_SIZE nor less
than NTFS_BLOCK_SIZE.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: linux-ntfs-dev@lists.sourceforge.net
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/ntfs/aops.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
index 3a2e509c77c5..58dadff3e0e0 100644
--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -926,7 +926,7 @@ static int ntfs_write_mst_block(struct page *page,
ntfs_volume *vol = ni->vol;
u8 *kaddr;
unsigned int rec_size = ni->itype.index.block_size;
- ntfs_inode *locked_nis[PAGE_SIZE / rec_size];
+ ntfs_inode *locked_nis[PAGE_SIZE / NTFS_BLOCK_SIZE];
struct buffer_head *bh, *head, *tbh, *rec_start_bh;
struct buffer_head *bhs[MAX_BUF_PER_PAGE];
runlist_element *rl;
@@ -935,6 +935,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;
+ if (WARN_ON(rec_size < NTFS_BLOCK_SIZE))
+ return -EINVAL;
+
ntfs_debug("Entering for inode 0x%lx, attribute type 0x%x, page index "
"0x%lx.", vi->i_ino, ni->type, page->index);
BUG_ON(!NInoNonResident(ni));
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] NTFS: decompress: Remove VLA usage
2018-06-26 17:29 [PATCH 0/3] NTFS: Remove VLA usage Kees Cook
2018-06-26 17:29 ` [PATCH 1/3] NTFS: aops: " Kees Cook
@ 2018-06-26 17:29 ` Kees Cook
2018-06-26 17:29 ` [PATCH 3/3] NTFS: mft: " Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2018-06-26 17:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Anton Altaparmakov, linux-ntfs-dev, Al Viro, Jan Kara,
linux-kernel
In the quest to remove all stack VLA usage from the kernel[1], this moves
the stack buffer used during decompression to be allocated externally.
The existing "dest_max_index" used in the VLA is bounded by cb_max_page.
cb_max_page is bounded by max_page, and max_page is bounded by nr_pages.
Since nr_pages is used for the "pages" allocation, it can similarly
be used for the "completed_pages" allocation and passed into the
decompression function. The error paths are updated to free the new
allocation.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: linux-ntfs-dev@lists.sourceforge.net
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/ntfs/compress.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
index fbd0090d7d0c..df7c32b5fac7 100644
--- a/fs/ntfs/compress.c
+++ b/fs/ntfs/compress.c
@@ -128,6 +128,7 @@ static inline void handle_bounds_compressed_page(struct page *page,
/**
* ntfs_decompress - decompress a compression block into an array of pages
* @dest_pages: destination array of pages
+ * @completed_pages: scratch space to track completed pages
* @dest_index: current index into @dest_pages (IN/OUT)
* @dest_ofs: current offset within @dest_pages[@dest_index] (IN/OUT)
* @dest_max_index: maximum index into @dest_pages (IN)
@@ -162,10 +163,10 @@ static inline void handle_bounds_compressed_page(struct page *page,
* Note to hackers: This function may not sleep until it has finished accessing
* the compression block @cb_start as it is a per-CPU buffer.
*/
-static int ntfs_decompress(struct page *dest_pages[], int *dest_index,
- int *dest_ofs, const int dest_max_index, const int dest_max_ofs,
- const int xpage, char *xpage_done, u8 *const cb_start,
- const u32 cb_size, const loff_t i_size,
+static int ntfs_decompress(struct page *dest_pages[], int completed_pages[],
+ int *dest_index, int *dest_ofs, const int dest_max_index,
+ const int dest_max_ofs, const int xpage, char *xpage_done,
+ u8 *const cb_start, const u32 cb_size, const loff_t i_size,
const s64 initialized_size)
{
/*
@@ -190,9 +191,6 @@ static int ntfs_decompress(struct page *dest_pages[], int *dest_index,
/* Variables for tag and token parsing. */
u8 tag; /* Current tag. */
int token; /* Loop counter for the eight tokens in tag. */
-
- /* Need this because we can't sleep, so need two stages. */
- int completed_pages[dest_max_index - *dest_index + 1];
int nr_completed_pages = 0;
/* Default error code. */
@@ -516,6 +514,7 @@ int ntfs_read_compressed_block(struct page *page)
unsigned int cb_clusters, cb_max_ofs;
int block, max_block, cb_max_page, bhs_size, nr_bhs, err = 0;
struct page **pages;
+ int *completed_pages;
unsigned char xpage_done = 0;
ntfs_debug("Entering, page->index = 0x%lx, cb_size = 0x%x, nr_pages = "
@@ -528,14 +527,16 @@ int ntfs_read_compressed_block(struct page *page)
BUG_ON(ni->name_len);
pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_NOFS);
+ completed_pages = kmalloc_array(nr_pages + 1, sizeof(int), GFP_NOFS);
/* Allocate memory to store the buffer heads we need. */
bhs_size = cb_size / block_size * sizeof(struct buffer_head *);
bhs = kmalloc(bhs_size, GFP_NOFS);
- if (unlikely(!pages || !bhs)) {
+ if (unlikely(!pages || !bhs || !completed_pages)) {
kfree(bhs);
kfree(pages);
+ kfree(completed_pages);
unlock_page(page);
ntfs_error(vol->sb, "Failed to allocate internal buffers.");
return -ENOMEM;
@@ -562,6 +563,7 @@ int ntfs_read_compressed_block(struct page *page)
if (xpage >= max_page) {
kfree(bhs);
kfree(pages);
+ kfree(completed_pages);
zero_user(page, 0, PAGE_SIZE);
ntfs_debug("Compressed read outside i_size - truncated?");
SetPageUptodate(page);
@@ -854,10 +856,10 @@ int ntfs_read_compressed_block(struct page *page)
unsigned int prev_cur_page = cur_page;
ntfs_debug("Found compressed compression block.");
- err = ntfs_decompress(pages, &cur_page, &cur_ofs,
- cb_max_page, cb_max_ofs, xpage, &xpage_done,
- cb_pos, cb_size - (cb_pos - cb), i_size,
- initialized_size);
+ err = ntfs_decompress(pages, completed_pages, &cur_page,
+ &cur_ofs, cb_max_page, cb_max_ofs, xpage,
+ &xpage_done, cb_pos, cb_size - (cb_pos - cb),
+ i_size, initialized_size);
/*
* We can sleep from now on, lock already dropped by
* ntfs_decompress().
@@ -912,6 +914,7 @@ int ntfs_read_compressed_block(struct page *page)
/* We no longer need the list of pages. */
kfree(pages);
+ kfree(completed_pages);
/* If we have completed the requested page, we return success. */
if (likely(xpage_done))
@@ -956,5 +959,6 @@ int ntfs_read_compressed_block(struct page *page)
}
}
kfree(pages);
+ kfree(completed_pages);
return -EIO;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] NTFS: mft: Remove VLA usage
2018-06-26 17:29 [PATCH 0/3] NTFS: Remove VLA usage Kees Cook
2018-06-26 17:29 ` [PATCH 1/3] NTFS: aops: " Kees Cook
2018-06-26 17:29 ` [PATCH 2/3] NTFS: decompress: " Kees Cook
@ 2018-06-26 17:29 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2018-06-26 17:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Anton Altaparmakov, linux-ntfs-dev, Al Viro, Jan Kara,
linux-kernel
In the quest to remove all stack VLA usage from the kernel[1], this
allocates the maximum size stack buffer. Existing checks already require
that blocksize >= NTFS_BLOCK_SIZE and mft_record_size <= PAGE_SIZE, so
max_bhs can be at most PAGE_SIZE / NTFS_BLOCK_SIZE. Sanity checks are
added for robustness.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: linux-ntfs-dev@lists.sourceforge.net
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/ntfs/mft.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 32c523cf5a2d..fb14d17666c8 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -35,6 +35,8 @@
#include "mft.h"
#include "ntfs.h"
+#define MAX_BHS (PAGE_SIZE / NTFS_BLOCK_SIZE)
+
/**
* map_mft_record_page - map the page in which a specific mft record resides
* @ni: ntfs inode whose mft record page to map
@@ -469,7 +471,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
struct page *page;
unsigned int blocksize = vol->sb->s_blocksize;
int max_bhs = vol->mft_record_size / blocksize;
- struct buffer_head *bhs[max_bhs];
+ struct buffer_head *bhs[MAX_BHS];
struct buffer_head *bh, *head;
u8 *kmirr;
runlist_element *rl;
@@ -479,6 +481,8 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
ntfs_debug("Entering for inode 0x%lx.", mft_no);
BUG_ON(!max_bhs);
+ if (WARN_ON(max_bhs > MAX_BHS))
+ return -EINVAL;
if (unlikely(!vol->mftmirr_ino)) {
/* This could happen during umount... */
err = ntfs_sync_mft_mirror_umount(vol, mft_no, m);
@@ -674,7 +678,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD *m, int sync)
unsigned int blocksize = vol->sb->s_blocksize;
unsigned char blocksize_bits = vol->sb->s_blocksize_bits;
int max_bhs = vol->mft_record_size / blocksize;
- struct buffer_head *bhs[max_bhs];
+ struct buffer_head *bhs[MAX_BHS];
struct buffer_head *bh, *head;
runlist_element *rl;
unsigned int block_start, block_end, m_start, m_end;
@@ -684,6 +688,10 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD *m, int sync)
BUG_ON(NInoAttr(ni));
BUG_ON(!max_bhs);
BUG_ON(!PageLocked(page));
+ if (WARN_ON(max_bhs > MAX_BHS)) {
+ err = -EINVAL;
+ goto err_out;
+ }
/*
* If the ntfs_inode is clean no need to do anything. If it is dirty,
* mark it as clean now so that it can be redirtied later on if needed.
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] NTFS: aops: Remove VLA usage
2018-06-26 17:29 ` [PATCH 1/3] NTFS: aops: " Kees Cook
@ 2018-07-05 20:44 ` Arnd Bergmann
2018-07-10 1:04 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2018-07-05 20:44 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Anton Altaparmakov, linux-ntfs-dev, Al Viro,
Jan Kara, Linux Kernel Mailing List
On Tue, Jun 26, 2018 at 7:29 PM, Kees Cook <keescook@chromium.org> wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the maximum size needed on the stack and adds a sanity check for
> robustness: index.block_size cannot be larger than PAGE_SIZE nor less
> than NTFS_BLOCK_SIZE.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Cc: Anton Altaparmakov <anton@tuxera.com>
> Cc: linux-ntfs-dev@lists.sourceforge.net
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/ntfs/aops.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index 3a2e509c77c5..58dadff3e0e0 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -926,7 +926,7 @@ static int ntfs_write_mst_block(struct page *page,
> ntfs_volume *vol = ni->vol;
> u8 *kaddr;
> unsigned int rec_size = ni->itype.index.block_size;
> - ntfs_inode *locked_nis[PAGE_SIZE / rec_size];
> + ntfs_inode *locked_nis[PAGE_SIZE / NTFS_BLOCK_SIZE];
> struct buffer_head *bh, *head, *tbh, *rec_start_bh;
> struct buffer_head *bhs[MAX_BUF_PER_PAGE];
> runlist_element *rl;
This has uncovered what looks like a preexisting bug on architectures
with large page size, this is what I get with 64K pages on arm64:
fs/ntfs/aops.c: In function 'ntfs_write_mst_block':
fs/ntfs/aops.c:1328:1: error: the frame size of 2432 bytes is larger
than 2048 bytes [-Werror=frame-larger-than=]
Since both ntfs and 64k pages are fairly obscure features, we might
get away with just disabling the combination of the two in Kconfig.
Using dynamic allocation might be tricky here, since I assume this
could be called during writeback in order to free memory, and I can't
immediately see any better fix.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] NTFS: aops: Remove VLA usage
2018-07-05 20:44 ` Arnd Bergmann
@ 2018-07-10 1:04 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2018-07-10 1:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andrew Morton, Anton Altaparmakov, linux-ntfs-dev, Al Viro,
Jan Kara, Linux Kernel Mailing List
On Thu, Jul 5, 2018 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 26, 2018 at 7:29 PM, Kees Cook <keescook@chromium.org> wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> uses the maximum size needed on the stack and adds a sanity check for
>> robustness: index.block_size cannot be larger than PAGE_SIZE nor less
>> than NTFS_BLOCK_SIZE.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Cc: Anton Altaparmakov <anton@tuxera.com>
>> Cc: linux-ntfs-dev@lists.sourceforge.net
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> fs/ntfs/aops.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
>> index 3a2e509c77c5..58dadff3e0e0 100644
>> --- a/fs/ntfs/aops.c
>> +++ b/fs/ntfs/aops.c
>> @@ -926,7 +926,7 @@ static int ntfs_write_mst_block(struct page *page,
>> ntfs_volume *vol = ni->vol;
>> u8 *kaddr;
>> unsigned int rec_size = ni->itype.index.block_size;
>> - ntfs_inode *locked_nis[PAGE_SIZE / rec_size];
>> + ntfs_inode *locked_nis[PAGE_SIZE / NTFS_BLOCK_SIZE];
>> struct buffer_head *bh, *head, *tbh, *rec_start_bh;
>> struct buffer_head *bhs[MAX_BUF_PER_PAGE];
>> runlist_element *rl;
>
> This has uncovered what looks like a preexisting bug on architectures
> with large page size, this is what I get with 64K pages on arm64:
>
> fs/ntfs/aops.c: In function 'ntfs_write_mst_block':
> fs/ntfs/aops.c:1328:1: error: the frame size of 2432 bytes is larger
> than 2048 bytes [-Werror=frame-larger-than=]
>
> Since both ntfs and 64k pages are fairly obscure features, we might
> get away with just disabling the combination of the two in Kconfig.
>
> Using dynamic allocation might be tricky here, since I assume this
> could be called during writeback in order to free memory, and I can't
> immediately see any better fix.
I'm open to whatever. In crypto, my series uses specifically 4096 for
PAGE_SIZE instead of using PAGE_SIZE, since it wasn't really related.
Here, though, I can't tell if it really IS a page size issue.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-10 1:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 17:29 [PATCH 0/3] NTFS: Remove VLA usage Kees Cook
2018-06-26 17:29 ` [PATCH 1/3] NTFS: aops: " Kees Cook
2018-07-05 20:44 ` Arnd Bergmann
2018-07-10 1:04 ` Kees Cook
2018-06-26 17:29 ` [PATCH 2/3] NTFS: decompress: " Kees Cook
2018-06-26 17:29 ` [PATCH 3/3] NTFS: mft: " Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).