[PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[Jeremy Cline]

'type' is a user-controlled value used to index into 's_qf_names', which
can be used in a Spectre v1 attack. Clamp 'type' to the size of the
array to avoid a speculative out-of-bounds read.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 fs/ext4/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6480e763080f..c04a09b51742 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -40,6 +40,7 @@
 #include <linux/crc16.h>
 #include <linux/dax.h>
 #include <linux/cleancache.h>
+#include <linux/nospec.h>
 #include <linux/uaccess.h>
 #include <linux/iversion.h>
 
@@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
 	if (path->dentry->d_sb != sb)
 		return -EXDEV;
 	/* Journaling quota? */
+	type = array_index_nospec(type, EXT4_MAXQUOTAS);
 	if (EXT4_SB(sb)->s_qf_names[type]) {
 		/* Quotafile not in fs root? */
 		if (path->dentry->d_parent != sb->s_root)
-- 
2.17.1

Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[Josh Poimboeuf]

On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
> 'type' is a user-controlled value used to index into 's_qf_names', which
> can be used in a Spectre v1 attack. Clamp 'type' to the size of the
> array to avoid a speculative out-of-bounds read.
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeremy Cline <jcline@redhat.com>
> ---
>  fs/ext4/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6480e763080f..c04a09b51742 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -40,6 +40,7 @@
>  #include <linux/crc16.h>
>  #include <linux/dax.h>
>  #include <linux/cleancache.h>
> +#include <linux/nospec.h>
>  #include <linux/uaccess.h>
>  #include <linux/iversion.h>
>  
> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>  	if (path->dentry->d_sb != sb)
>  		return -EXDEV;
>  	/* Journaling quota? */
> +	type = array_index_nospec(type, EXT4_MAXQUOTAS);
>  	if (EXT4_SB(sb)->s_qf_names[type]) {
>  		/* Quotafile not in fs root? */
>  		if (path->dentry->d_parent != sb->s_root)

Generally we try to put the array_index_nospec() close to the bounds
check for which it's trying to prevent speculation past.

In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in
do_quotactl(), but it seems to be missing:

	if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
		return -EINVAL;

Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have
the same value (3).  Maybe they can be consolidated to just use
MAXQUOTAS everywhere?  Then the nospec would be simple:

	if (type >= MAXQUOTAS)
		return -EINVAL;
	type = array_index_nospec(type, MAXQUOTAS);

Otherwise I think we may need to disperse the array_index_nospec calls
deeper in the callchain.

-- 
Josh

Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[Jeremy Cline]

On 07/27/2018 01:46 PM, Josh Poimboeuf wrote:
> On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
>> 'type' is a user-controlled value used to index into 's_qf_names', which
>> can be used in a Spectre v1 attack. Clamp 'type' to the size of the
>> array to avoid a speculative out-of-bounds read.
>>
>> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jeremy Cline <jcline@redhat.com>
>> ---
>>  fs/ext4/super.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 6480e763080f..c04a09b51742 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -40,6 +40,7 @@
>>  #include <linux/crc16.h>
>>  #include <linux/dax.h>
>>  #include <linux/cleancache.h>
>> +#include <linux/nospec.h>
>>  #include <linux/uaccess.h>
>>  #include <linux/iversion.h>
>>  
>> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>>  	if (path->dentry->d_sb != sb)
>>  		return -EXDEV;
>>  	/* Journaling quota? */
>> +	type = array_index_nospec(type, EXT4_MAXQUOTAS);
>>  	if (EXT4_SB(sb)->s_qf_names[type]) {
>>  		/* Quotafile not in fs root? */
>>  		if (path->dentry->d_parent != sb->s_root)
> 
> Generally we try to put the array_index_nospec() close to the bounds
> check for which it's trying to prevent speculation past.
> 
> In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in
> do_quotactl(), but it seems to be missing:
> 
> 	if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
> 		return -EINVAL;
> 
> Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have
> the same value (3).  Maybe they can be consolidated to just use
> MAXQUOTAS everywhere?  Then the nospec would be simple:
> 
> 	if (type >= MAXQUOTAS)
> 		return -EINVAL;
> 	type = array_index_nospec(type, MAXQUOTAS);
> 
> Otherwise I think we may need to disperse the array_index_nospec calls
> deeper in the callchain.
> 

Makes sense, that would be much nicer. I looked into the history a bit,
EXT4_MAXQUOTAS was adjusted from 2 to 3 in v4.5 so a backport of this to
v4.4 or earlier would require a bit of adjustment, but XQM_MAXQUOTAS
looks to have been 3 for a very long time.

I'll see about consolidating them and adjusting this patch.

Thanks,
Jeremy

Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2813 bytes --]


> On Jul 27, 2018, at 11:46 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
>> 'type' is a user-controlled value used to index into 's_qf_names', which
>> can be used in a Spectre v1 attack. Clamp 'type' to the size of the
>> array to avoid a speculative out-of-bounds read.
>> 
>> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jeremy Cline <jcline@redhat.com>
>> ---
>> fs/ext4/super.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 6480e763080f..c04a09b51742 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -40,6 +40,7 @@
>> #include <linux/crc16.h>
>> #include <linux/dax.h>
>> #include <linux/cleancache.h>
>> +#include <linux/nospec.h>
>> #include <linux/uaccess.h>
>> #include <linux/iversion.h>
>> 
>> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>> 	if (path->dentry->d_sb != sb)
>> 		return -EXDEV;
>> 	/* Journaling quota? */
>> +	type = array_index_nospec(type, EXT4_MAXQUOTAS);

This check just papers over the issue, but AFAICS doesn't actually
solve any problems.  It ends up squashing an invalid value to be
the same as EXT4_MAXQUOTAS, rather than returning an error to the
caller as it should.

>> 	if (EXT4_SB(sb)->s_qf_names[type]) {
>> 		/* Quotafile not in fs root? */
>> 		if (path->dentry->d_parent != sb->s_root)
> 
> Generally we try to put the array_index_nospec() close to the bounds
> check for which it's trying to prevent speculation past.
> 
> In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in
> do_quotactl(), but it seems to be missing:
> 
> 	if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
> 		return -EINVAL;

Agreed that this should be checked at the highest layer possible.
IMHO, this means one check in the VFS/quota layer, and a separate
check in the filesystem layer.

> Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have the same value (3).  Maybe they can be consolidated to just use
> MAXQUOTAS everywhere?

No, the filesystem-specific MAXQUOTAS values were separated from
the kernel MAXQUOTAS value for a good reason.  This allows some
filesystems to support new quota types (e.g. project quotas) that
not all other filesystems can handle.  This may potentially change
again in the future, so they shouldn't be tightly coupled.

>  Then the nospec would be simple:
> 
> 	if (type >= MAXQUOTAS)
> 		return -EINVAL;
> 	type = array_index_nospec(type, MAXQUOTAS);
> 
> Otherwise I think we may need to disperse the array_index_nospec calls
> deeper in the callchain.
> 
> --
> Josh


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/3] ext4: super: Fix spectre gadget in ext4_quota_on

[Josh Poimboeuf]

On Tue, Jul 31, 2018 at 12:39:41AM -0600, Andreas Dilger wrote:
> > Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all
> > have the same value (3).  Maybe they can be consolidated to just use
> > MAXQUOTAS everywhere?
> 
> No, the filesystem-specific MAXQUOTAS values were separated from
> the kernel MAXQUOTAS value for a good reason.  This allows some
> filesystems to support new quota types (e.g. project quotas) that
> not all other filesystems can handle.  This may potentially change
> again in the future, so they shouldn't be tightly coupled.

But isn't that what sb->s_quota_types is for?  To allow different
filesystems to support different quota types?

Also I don't see any bounds checks for EXT4_MAXQUOTAS.  It seems like
the ext4 code assumes that MAXQUOTAS and EXT4_MAXQUOTAS are the same.

-- 
Josh

[PATCH 2/3] ext4: super: Fix spectre gadgets in ext4_quota_{read,write,off}

[Jeremy Cline]

'type' is a user-controlled value used to index 'sb_dqopt(sb)->files'.
Clamp 'type' to the size of the array to avoid a speculative
out-of-bounds read.

These gadgets were found with the help of smatch:

* fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]

* fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 fs/ext4/super.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c04a09b51742..de358eba024a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5693,10 +5693,13 @@ static int ext4_enable_quotas(struct super_block *sb)
 
 static int ext4_quota_off(struct super_block *sb, int type)
 {
-	struct inode *inode = sb_dqopt(sb)->files[type];
+	struct inode *inode;
 	handle_t *handle;
 	int err;
 
+	type = array_index_nospec(type, MAXQUOTAS);
+	inode = sb_dqopt(sb)->files[type];
+
 	/* Force all delayed allocation blocks to be allocated.
 	 * Caller already holds s_umount sem */
 	if (test_opt(sb, DELALLOC))
@@ -5740,13 +5743,17 @@ static int ext4_quota_off(struct super_block *sb, int type)
 static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
 			       size_t len, loff_t off)
 {
-	struct inode *inode = sb_dqopt(sb)->files[type];
+	struct inode *inode;
 	ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
 	int offset = off & (sb->s_blocksize - 1);
 	int tocopy;
 	size_t toread;
 	struct buffer_head *bh;
-	loff_t i_size = i_size_read(inode);
+	loff_t i_size;
+
+	type = array_index_nospec(type, MAXQUOTAS);
+	inode = sb_dqopt(sb)->files[type];
+	i_size = i_size_read(inode);
 
 	if (off > i_size)
 		return 0;
@@ -5777,13 +5784,16 @@ static ssize_t ext4_quota_read(struct super_block *sb, int type, char *data,
 static ssize_t ext4_quota_write(struct super_block *sb, int type,
 				const char *data, size_t len, loff_t off)
 {
-	struct inode *inode = sb_dqopt(sb)->files[type];
+	struct inode *inode;
 	ext4_lblk_t blk = off >> EXT4_BLOCK_SIZE_BITS(sb);
 	int err, offset = off & (sb->s_blocksize - 1);
 	int retries = 0;
 	struct buffer_head *bh;
 	handle_t *handle = journal_current_handle();
 
+	type = array_index_nospec(type, MAXQUOTAS);
+	inode = sb_dqopt(sb)->files[type];
+
 	if (EXT4_SB(sb)->s_journal && !handle) {
 		ext4_msg(sb, KERN_WARNING, "Quota write (off=%llu, len=%llu)"
 			" cancelled because transaction is not started",
-- 
2.17.1

[PATCH 3/3] ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group

[Jeremy Cline]

'ac->ac_2order' is a user-controlled value used to index into
'grp->bb_counters' and based on the value at that index, 'ac->ac_found'
is written to. Clamp the value right after the bounds check to avoid a
speculative out-of-bounds read of 'grp->bb_counters'.

This also protects the access of the s_mb_offsets and s_mb_maxs arrays
inside mb_find_buddy().

These gadgets were discovered with the help of smatch:

* fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential
  spectre issue 'grp->bb_counters' [w] (local cap)

* fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue
  'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap)

* fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue
  'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap)

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 fs/ext4/mballoc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f7ab34088162..c0866007a949 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -14,6 +14,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/nospec.h>
 #include <linux/backing-dev.h>
 #include <trace/events/ext4.h>
 
@@ -1893,6 +1894,7 @@ void ext4_mb_simple_scan_group(struct ext4_allocation_context *ac,
 
 	BUG_ON(ac->ac_2order <= 0);
 	for (i = ac->ac_2order; i <= sb->s_blocksize_bits + 1; i++) {
+		i = array_index_nospec(i, sb->s_blocksize_bits + 2);
 		if (grp->bb_counters[i] == 0)
 			continue;
 
-- 
2.17.1

Re: [PATCH 3/3] ext4: mballoc: Fix spectre gadget in ext4_mb_simple_scan_group

[Josh Poimboeuf]

On Fri, Jul 27, 2018 at 04:23:57PM +0000, Jeremy Cline wrote:
> 'ac->ac_2order' is a user-controlled value used to index into
> 'grp->bb_counters' and based on the value at that index, 'ac->ac_found'
> is written to. Clamp the value right after the bounds check to avoid a
> speculative out-of-bounds read of 'grp->bb_counters'.
> 
> This also protects the access of the s_mb_offsets and s_mb_maxs arrays
> inside mb_find_buddy().
> 
> These gadgets were discovered with the help of smatch:
> 
> * fs/ext4/mballoc.c:1896 ext4_mb_simple_scan_group() warn: potential
>   spectre issue 'grp->bb_counters' [w] (local cap)
> 
> * fs/ext4/mballoc.c:445 mb_find_buddy() warn: potential spectre issue
>   'EXT4_SB(e4b->bd_sb)->s_mb_offsets' [r] (local cap)
> 
> * fs/ext4/mballoc.c:446 mb_find_buddy() warn: potential spectre issue
>   'EXT4_SB(e4b->bd_sb)->s_mb_maxs' [r] (local cap)
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeremy Cline <jcline@redhat.com>
> ---
>  fs/ext4/mballoc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index f7ab34088162..c0866007a949 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -14,6 +14,7 @@
>  #include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/nospec.h>
>  #include <linux/backing-dev.h>
>  #include <trace/events/ext4.h>
>  
> @@ -1893,6 +1894,7 @@ void ext4_mb_simple_scan_group(struct ext4_allocation_context *ac,
>  
>  	BUG_ON(ac->ac_2order <= 0);
>  	for (i = ac->ac_2order; i <= sb->s_blocksize_bits + 1; i++) {
> +		i = array_index_nospec(i, sb->s_blocksize_bits + 2);
>  		if (grp->bb_counters[i] == 0)
>  			continue;

Similar to my patch 1 comment, it's better to go up the call chain.

ac_2order's user taint seems to come from ext4_mb_regular_allocator(),
where it's derived from ac->ac_g_ex.fe_len, which has a user taint
according to smatch.

	i = fls(ac->ac_g_ex.fe_len);
	ac->ac_2order = 0;
	/*
	 * We search using buddy data only if the order of the request
	 * is greater than equal to the sbi_s_mb_order2_reqs
	 * You can tune it via /sys/fs/ext4/<partition>/mb_order2_req
	 * We also support searching for power-of-two requests only for
	 * requests upto maximum buddy size we have constructed.
	 */
	if (i >= sbi->s_mb_order2_reqs && i <= sb->s_blocksize_bits + 2) {
		/*
		 * This should tell if fe_len is exactly power of 2
		 */
		if ((ac->ac_g_ex.fe_len & (~(1 << (i - 1)))) == 0)
			ac->ac_2order = i - 1;
	}

So here maybe you could change the assignment to:

			ac->ac_2order = array_index_nospec(i - 1,
							   sb->s_blocksize_bits + 2);

That makes it easier for a reader of the code to understand what
speculation we're protecting against.  And it also protects other
consumers of this value down the call chain.

-- 
Josh