[PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[Reinette Chatre]

While the DOC at the beginning of lib/bitmap.c explicitly states that
"The number of valid bits in a given bitmap does _not_ need to be an
exact multiple of BITS_PER_LONG.", some of the bitmap operations do
indeed access BITS_PER_LONG portions of the provided bitmap no matter
the size of the provided bitmap. For example, if find_first_bit()
is provided with an 8 bit bitmap the operation will access
BITS_PER_LONG bits from the provided bitmap. While the operation
ensures that these extra bits do not affect the result, the memory
is still accessed.

The capacity bitmasks (CBMs) are typically stored in u32 since they
can never exceed 32 bits. A few instances exist where a bitmap_*
operation is performed on a CBM by simply pointing the bitmap operation
to the stored u32 value.

The consequence of this pattern is that some bitmap_* operations will
access out-of-bounds memory when interacting with the provided CBM.

This same issue has previously been addressed with commit 49e00eee0061
("x86/intel_rdt: Fix out-of-bounds memory access in CBM tests")
but at that time not all instances of the issue were fixed.

Fix this by using an unsigned long to store the capacity bitmask data
that is passed to bitmap functions.

Fixes: e651901187ab ("x86/intel_rdt: Introduce "bit_usage" to display cache allocations details")
Fixes: f4e80d67a527 ("x86/intel_rdt: Resctrl files reflect pseudo-locked information")
Fixes: 95f0b77efa57 ("x86/intel_rdt: Initialize new resource group with sane defaults")

Cc: <stable@vger.kernel.org>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 35 ++++++++++++--------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 333c177a2471..b63e50b1a096 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -804,8 +804,12 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
 			      struct seq_file *seq, void *v)
 {
 	struct rdt_resource *r = of->kn->parent->priv;
-	u32 sw_shareable = 0, hw_shareable = 0;
-	u32 exclusive = 0, pseudo_locked = 0;
+	/*
+	 * Use unsigned long even though only 32 bits are used to ensure
+	 * test_bit() is used safely.
+	 */
+	unsigned long sw_shareable = 0, hw_shareable = 0;
+	unsigned long exclusive = 0, pseudo_locked = 0;
 	struct rdt_domain *dom;
 	int i, hwb, swb, excl, psl;
 	enum rdtgrp_mode mode;
@@ -850,10 +854,10 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
 		}
 		for (i = r->cache.cbm_len - 1; i >= 0; i--) {
 			pseudo_locked = dom->plr ? dom->plr->cbm : 0;
-			hwb = test_bit(i, (unsigned long *)&hw_shareable);
-			swb = test_bit(i, (unsigned long *)&sw_shareable);
-			excl = test_bit(i, (unsigned long *)&exclusive);
-			psl = test_bit(i, (unsigned long *)&pseudo_locked);
+			hwb = test_bit(i, &hw_shareable);
+			swb = test_bit(i, &sw_shareable);
+			excl = test_bit(i, &exclusive);
+			psl = test_bit(i, &pseudo_locked);
 			if (hwb && swb)
 				seq_putc(seq, 'X');
 			else if (hwb && !swb)
@@ -2494,26 +2498,19 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
  */
 static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r)
 {
-	/*
-	 * Convert the u32 _val to an unsigned long required by all the bit
-	 * operations within this function. No more than 32 bits of this
-	 * converted value can be accessed because all bit operations are
-	 * additionally provided with cbm_len that is initialized during
-	 * hardware enumeration using five bits from the EAX register and
-	 * thus never can exceed 32 bits.
-	 */
-	unsigned long *val = (unsigned long *)_val;
+	unsigned long val = *_val;
 	unsigned int cbm_len = r->cache.cbm_len;
 	unsigned long first_bit, zero_bit;
 
-	if (*val == 0)
+	if (val == 0)
 		return;
 
-	first_bit = find_first_bit(val, cbm_len);
-	zero_bit = find_next_zero_bit(val, cbm_len, first_bit);
+	first_bit = find_first_bit(&val, cbm_len);
+	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
 
 	/* Clear any remaining bits to ensure contiguous region */
-	bitmap_clear(val, zero_bit, cbm_len - zero_bit);
+	bitmap_clear(&val, zero_bit, cbm_len - zero_bit);
+	*_val = (u32)val;
 }
 
 /*
-- 
2.17.2

Re: [PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[Borislav Petkov]

On Wed, Jun 19, 2019 at 01:27:16PM -0700, Reinette Chatre wrote:
> @@ -2494,26 +2498,19 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>   */
>  static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r)
>  {
> -	/*
> -	 * Convert the u32 _val to an unsigned long required by all the bit
> -	 * operations within this function. No more than 32 bits of this
> -	 * converted value can be accessed because all bit operations are
> -	 * additionally provided with cbm_len that is initialized during
> -	 * hardware enumeration using five bits from the EAX register and
> -	 * thus never can exceed 32 bits.
> -	 */
> -	unsigned long *val = (unsigned long *)_val;
> +	unsigned long val = *_val;
>  	unsigned int cbm_len = r->cache.cbm_len;
>  	unsigned long first_bit, zero_bit;

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

> -	if (*val == 0)
> +	if (val == 0)

	if (!val)

>  		return;
>  
> -	first_bit = find_first_bit(val, cbm_len);
> -	zero_bit = find_next_zero_bit(val, cbm_len, first_bit);
> +	first_bit = find_first_bit(&val, cbm_len);
> +	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>  
>  	/* Clear any remaining bits to ensure contiguous region */
> -	bitmap_clear(val, zero_bit, cbm_len - zero_bit);
> +	bitmap_clear(&val, zero_bit, cbm_len - zero_bit);
> +	*_val = (u32)val;

... and also, that function should simply return the u32 value instead
of using @_val as an input and output var.

But that should be a separate cleanup patch anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip:x86/urgent] x86/resctrl: Prevent possible overrun during bitmap operations

[tip-bot for Reinette Chatre]

Commit-ID:  32f010deab575199df4ebe7b6aec20c17bb7eccd
Gitweb:     https://git.kernel.org/tip/32f010deab575199df4ebe7b6aec20c17bb7eccd
Author:     Reinette Chatre <reinette.chatre@intel.com>
AuthorDate: Wed, 19 Jun 2019 13:27:16 -0700
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Thu, 20 Jun 2019 15:39:19 +0200

x86/resctrl: Prevent possible overrun during bitmap operations

While the DOC at the beginning of lib/bitmap.c explicitly states that
"The number of valid bits in a given bitmap does _not_ need to be an
exact multiple of BITS_PER_LONG.", some of the bitmap operations do
indeed access BITS_PER_LONG portions of the provided bitmap no matter
the size of the provided bitmap.

For example, if find_first_bit() is provided with an 8 bit bitmap the
operation will access BITS_PER_LONG bits from the provided bitmap. While
the operation ensures that these extra bits do not affect the result,
the memory is still accessed.

The capacity bitmasks (CBMs) are typically stored in u32 since they
can never exceed 32 bits. A few instances exist where a bitmap_*
operation is performed on a CBM by simply pointing the bitmap operation
to the stored u32 value.

The consequence of this pattern is that some bitmap_* operations will
access out-of-bounds memory when interacting with the provided CBM.

This same issue has previously been addressed with commit 49e00eee0061
("x86/intel_rdt: Fix out-of-bounds memory access in CBM tests")
but at that time not all instances of the issue were fixed.

Fix this by using an unsigned long to store the capacity bitmask data
that is passed to bitmap functions.

Fixes: e651901187ab ("x86/intel_rdt: Introduce "bit_usage" to display cache allocations details")
Fixes: f4e80d67a527 ("x86/intel_rdt: Resctrl files reflect pseudo-locked information")
Fixes: 95f0b77efa57 ("x86/intel_rdt: Initialize new resource group with sane defaults")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: stable <stable@vger.kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/58c9b6081fd9bf599af0dfc01a6fdd335768efef.1560975645.git.reinette.chatre@intel.com
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 35 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 869cbef5da81..f9d8ed6ab03b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -804,8 +804,12 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
 			      struct seq_file *seq, void *v)
 {
 	struct rdt_resource *r = of->kn->parent->priv;
-	u32 sw_shareable = 0, hw_shareable = 0;
-	u32 exclusive = 0, pseudo_locked = 0;
+	/*
+	 * Use unsigned long even though only 32 bits are used to ensure
+	 * test_bit() is used safely.
+	 */
+	unsigned long sw_shareable = 0, hw_shareable = 0;
+	unsigned long exclusive = 0, pseudo_locked = 0;
 	struct rdt_domain *dom;
 	int i, hwb, swb, excl, psl;
 	enum rdtgrp_mode mode;
@@ -850,10 +854,10 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
 		}
 		for (i = r->cache.cbm_len - 1; i >= 0; i--) {
 			pseudo_locked = dom->plr ? dom->plr->cbm : 0;
-			hwb = test_bit(i, (unsigned long *)&hw_shareable);
-			swb = test_bit(i, (unsigned long *)&sw_shareable);
-			excl = test_bit(i, (unsigned long *)&exclusive);
-			psl = test_bit(i, (unsigned long *)&pseudo_locked);
+			hwb = test_bit(i, &hw_shareable);
+			swb = test_bit(i, &sw_shareable);
+			excl = test_bit(i, &exclusive);
+			psl = test_bit(i, &pseudo_locked);
 			if (hwb && swb)
 				seq_putc(seq, 'X');
 			else if (hwb && !swb)
@@ -2494,26 +2498,19 @@ out_destroy:
  */
 static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r)
 {
-	/*
-	 * Convert the u32 _val to an unsigned long required by all the bit
-	 * operations within this function. No more than 32 bits of this
-	 * converted value can be accessed because all bit operations are
-	 * additionally provided with cbm_len that is initialized during
-	 * hardware enumeration using five bits from the EAX register and
-	 * thus never can exceed 32 bits.
-	 */
-	unsigned long *val = (unsigned long *)_val;
+	unsigned long val = *_val;
 	unsigned int cbm_len = r->cache.cbm_len;
 	unsigned long first_bit, zero_bit;
 
-	if (*val == 0)
+	if (val == 0)
 		return;
 
-	first_bit = find_first_bit(val, cbm_len);
-	zero_bit = find_next_zero_bit(val, cbm_len, first_bit);
+	first_bit = find_first_bit(&val, cbm_len);
+	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
 
 	/* Clear any remaining bits to ensure contiguous region */
-	bitmap_clear(val, zero_bit, cbm_len - zero_bit);
+	bitmap_clear(&val, zero_bit, cbm_len - zero_bit);
+	*_val = (u32)val;
 }
 
 /*

Re: [PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[Reinette Chatre]

Hi Borislav,

On 6/20/2019 6:44 AM, Borislav Petkov wrote:
> On Wed, Jun 19, 2019 at 01:27:16PM -0700, Reinette Chatre wrote:
>> @@ -2494,26 +2498,19 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>>   */
>>  static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r)
>>  {
>> -	/*
>> -	 * Convert the u32 _val to an unsigned long required by all the bit
>> -	 * operations within this function. No more than 32 bits of this
>> -	 * converted value can be accessed because all bit operations are
>> -	 * additionally provided with cbm_len that is initialized during
>> -	 * hardware enumeration using five bits from the EAX register and
>> -	 * thus never can exceed 32 bits.
>> -	 */
>> -	unsigned long *val = (unsigned long *)_val;
>> +	unsigned long val = *_val;
>>  	unsigned int cbm_len = r->cache.cbm_len;
>>  	unsigned long first_bit, zero_bit;
> 
> Please sort function local variables declaration in a reverse christmas
> tree order:
> 
> 	<type A> longest_variable_name;
> 	<type B> shorter_var_name;
> 	<type C> even_shorter;
> 	<type D> i;
> 
>> -	if (*val == 0)
>> +	if (val == 0)
> 
> 	if (!val)
> 
>>  		return;
>>  
>> -	first_bit = find_first_bit(val, cbm_len);
>> -	zero_bit = find_next_zero_bit(val, cbm_len, first_bit);
>> +	first_bit = find_first_bit(&val, cbm_len);
>> +	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>>  
>>  	/* Clear any remaining bits to ensure contiguous region */
>> -	bitmap_clear(val, zero_bit, cbm_len - zero_bit);
>> +	bitmap_clear(&val, zero_bit, cbm_len - zero_bit);
>> +	*_val = (u32)val;
> 
> ... and also, that function should simply return the u32 value instead
> of using @_val as an input and output var.
> 
> But that should be a separate cleanup patch anyway.

Thank you very much. I will provide that separate cleanup patch.

Reinette

RE: [PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[David Laight]

From: Reinette Chatre
> Sent: 19 June 2019 21:27
> 
> While the DOC at the beginning of lib/bitmap.c explicitly states that
> "The number of valid bits in a given bitmap does _not_ need to be an
> exact multiple of BITS_PER_LONG.", some of the bitmap operations do
> indeed access BITS_PER_LONG portions of the provided bitmap no matter
> the size of the provided bitmap. For example, if find_first_bit()
> is provided with an 8 bit bitmap the operation will access
> BITS_PER_LONG bits from the provided bitmap. While the operation
> ensures that these extra bits do not affect the result, the memory
> is still accessed.

I suspect that comment also needs correcting.
On BE systems you really do need to have a array of longs.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/resctrl: Prevent possible overrun during bitmap operations

[Reinette Chatre]

Hi David,

On 6/24/2019 6:55 AM, David Laight wrote:
> From: Reinette Chatre
>> Sent: 19 June 2019 21:27
>>
>> While the DOC at the beginning of lib/bitmap.c explicitly states that
>> "The number of valid bits in a given bitmap does _not_ need to be an
>> exact multiple of BITS_PER_LONG.", some of the bitmap operations do
>> indeed access BITS_PER_LONG portions of the provided bitmap no matter
>> the size of the provided bitmap. For example, if find_first_bit()
>> is provided with an 8 bit bitmap the operation will access
>> BITS_PER_LONG bits from the provided bitmap. While the operation
>> ensures that these extra bits do not affect the result, the memory
>> is still accessed.
> 
> I suspect that comment also needs correcting.
> On BE systems you really do need to have a array of longs.
> 

Thank you very much for taking a look. I believe that the lib/bitmap.c
documentation is correct, it is me that misinterpreted it and to make
matters worse I only provided the portion I misinterpreted in my commit
message. Before the portion that I quote above it is stated clearly that
it is implemented using an array of unsigned longs.

Reinette