[PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Jeremy Cline]

'ac->ac_g_ex.fe_len' is a user-controlled value which is used in the
derivation of 'ac->ac_2order'. 'ac->ac_2order', in turn, is used to
index arrays which makes it a potential spectre gadget. Fix this by
sanitizing the value assigned to 'ac->ac2_order'.  This covers the
following accesses found 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
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---

I broke this out of the "ext4: fix spectre v1 gadgets" patch set since
the other patches in that series could, as Josh noted, be replaced with
one fix in do_quotactl. I'll send that fix to the disk quota folks
separately.

Changes from v1:
  - Sanitize ac_2order on assignment, rather than down the call chain in
    ext4_mb_simple_scan_group.

 fs/ext4/mballoc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f7ab34088162..8b24d3d42cb3 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>
 
@@ -2140,7 +2141,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 		 * 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;
+			ac->ac_2order = array_index_nospec(i - 1,
+							   sb->s_blocksize_bits + 2);
 	}
 
 	/* if stream allocation is enabled, use global goal */
-- 
2.17.1

Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Theodore Y. Ts'o]

Hey Jeremy,

I think you are also going to be changing the 1/3 patch from the
original patch series that this was part of.  That's correct, right?

It would be easier for me if you could simply make all of the
revisions you plan to make for the patch series, and then upload a
full v2 of the entire patch series.

Right now I've mentally marked the entire patch series as "waiting
forh v2".  So when you send a V2 version of individual patch out of
that series, it leaves things unclear when/if you plan to update any
of other patches in the series.

Thanks!!

						- Ted

Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Jeremy Cline]

Hi Ted,

On 07/30/2018 02:36 PM, Theodore Y. Ts'o wrote:
> Hey Jeremy,
> 
> I think you are also going to be changing the 1/3 patch from the
> original patch series that this was part of.  That's correct, right?
> 
> It would be easier for me if you could simply make all of the
> revisions you plan to make for the patch series, and then upload a
> full v2 of the entire patch series.
> 
> Right now I've mentally marked the entire patch series as "waiting
> forh v2".  So when you send a V2 version of individual patch out of
> that series, it leaves things unclear when/if you plan to update any
> of other patches in the series.

I dropped patch 1/3 and 2/3 from the original series because they can
both be covered by some sanitation in fs/quota/quota.c, so the this is
only patch from the v1 series that should be applied.

Sorry for not being more clear!

Cheers,
Jeremy

Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Josh Poimboeuf]

On Mon, Jul 30, 2018 at 06:07:47PM +0000, Jeremy Cline wrote:
> 'ac->ac_g_ex.fe_len' is a user-controlled value which is used in the
> derivation of 'ac->ac_2order'. 'ac->ac_2order', in turn, is used to
> index arrays which makes it a potential spectre gadget. Fix this by
> sanitizing the value assigned to 'ac->ac2_order'.  This covers the
> following accesses found 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
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Theodore Y. Ts'o]

On Mon, Jul 30, 2018 at 02:46:59PM -0400, Jeremy Cline wrote:
> I dropped patch 1/3 and 2/3 from the original series because they can
> both be covered by some sanitation in fs/quota/quota.c, so the this is
> only patch from the v1 series that should be applied.
> 
> Sorry for not being more clear!

Great, thanks for the clarification!  I'll review the patch posthaste!

      	      	      		      	   	  - Ted

Re: [PATCH v2] ext4: mballoc: Fix spectre gadget in ext4_mb_regular_allocator

[Theodore Y. Ts'o]

On Mon, Jul 30, 2018 at 06:07:47PM +0000, Jeremy Cline wrote:
> 'ac->ac_g_ex.fe_len' is a user-controlled value which is used in the
> derivation of 'ac->ac_2order'. 'ac->ac_2order', in turn, is used to
> index arrays which makes it a potential spectre gadget. Fix this by
> sanitizing the value assigned to 'ac->ac2_order'.  This covers the
> following accesses found 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
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

Thanks, applied.

					- Ted