diff mbox

[v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

Message ID 4EF3BEBA.4040402@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Xi Wang Dec. 22, 2011, 11:35 p.m. UTC 
Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
cause a kernel oops due to insufficient bounds checking.

	if (count > 1<<30) {
		/* Enforce a limit to prevent overflow */
		return -EINVAL;
	}
	count = roundup_pow_of_two(count);
	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));

Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:

	... + (count * sizeof(struct rps_dev_flow))

where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
32 bits.

This patch replaces the magic number (1 << 30) with a symbolic bound.

Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/core/net-sysfs.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

David Miller Dec. 23, 2011, 3:37 a.m. UTC | #1 
From: Xi Wang <xi.wang@gmail.com>
Date: Thu, 22 Dec 2011 18:35:22 -0500

> Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
> cause a kernel oops due to insufficient bounds checking.
> 
> 	if (count > 1<<30) {
> 		/* Enforce a limit to prevent overflow */
> 		return -EINVAL;
> 	}
> 	count = roundup_pow_of_two(count);
> 	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
> 
> Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:
> 
> 	... + (count * sizeof(struct rps_dev_flow))
> 
> where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
> 32 bits.
> 
> This patch replaces the magic number (1 << 30) with a symbolic bound.
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 23, 2011, 4:10 a.m. UTC | #2 
Le jeudi 22 décembre 2011 à 18:35 -0500, Xi Wang a écrit :
> Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
> cause a kernel oops due to insufficient bounds checking.
> 
> 	if (count > 1<<30) {
> 		/* Enforce a limit to prevent overflow */
> 		return -EINVAL;
> 	}
> 	count = roundup_pow_of_two(count);
> 	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
> 
> Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:
> 
> 	... + (count * sizeof(struct rps_dev_flow))
> 
> where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
> 32 bits.
> 
> This patch replaces the magic number (1 << 30) with a symbolic bound.
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/core/net-sysfs.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index c71c434..385aefe 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -665,11 +665,14 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
>  	if (count) {
>  		int i;
>  
> -		if (count > 1<<30) {
> +		if (count > INT_MAX)
> +			return -EINVAL;
> +		count = roundup_pow_of_two(count);
> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))

Oh well, you added a bug here, since count is "unsigned int"

Why mixing INT_MAX in the previous test and ULONG_MAX here ?


> +				/ sizeof(struct rps_dev_flow)) {
>  			/* Enforce a limit to prevent overflow */
>  			return -EINVAL;
>  		}
> -		count = roundup_pow_of_two(count);
>  		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
>  		if (!table)
>  			return -ENOMEM;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang Dec. 23, 2011, 4:44 a.m. UTC | #3 
On Dec 22, 2011, at 11:10 PM, Eric Dumazet wrote:
>> -		if (count > 1<<30) {
>> +		if (count > INT_MAX)
>> +			return -EINVAL;
>> +		count = roundup_pow_of_two(count);
>> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> 
> Oh well, you added a bug here, since count is "unsigned int"
> 
> Why mixing INT_MAX in the previous test and ULONG_MAX here ?

The first check (count > INT_MAX) is used to avoid overflowing
roundup_pow_of_two(count), e.g., when count is 0xffffffff.

The second check is to avoid overflowing the size for vmalloc().
It gives a less restrictive upper bound than using INT_MAX/UINT_MAX
on 64-bit platform.

Why do you think it's a bug there?  I agree using two checks looks
ugly though.

- xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 23, 2011, 4:53 a.m. UTC | #4 
Le jeudi 22 décembre 2011 à 23:44 -0500, Xi Wang a écrit :
> On Dec 22, 2011, at 11:10 PM, Eric Dumazet wrote:
> >> -		if (count > 1<<30) {
> >> +		if (count > INT_MAX)
> >> +			return -EINVAL;
> >> +		count = roundup_pow_of_two(count);
> >> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> > 
> > Oh well, you added a bug here, since count is "unsigned int"
> > 
> > Why mixing INT_MAX in the previous test and ULONG_MAX here ?
> 
> The first check (count > INT_MAX) is used to avoid overflowing
> roundup_pow_of_two(count), e.g., when count is 0xffffffff.
> 
> The second check is to avoid overflowing the size for vmalloc().
> It gives a less restrictive upper bound than using INT_MAX/UINT_MAX
> on 64-bit platform.
> 
> Why do you think it's a bug there?  I agree using two checks looks
> ugly though.
> 
> - xi

All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
have spotted the other bug in the code :

unsigned int count;

count = simple_strtoul(buf, &endp, 0);



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang Dec. 23, 2011, 5:10 a.m. UTC | #5 
On Dec 22, 2011, at 11:53 PM, Eric Dumazet wrote:
> All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
> have spotted the other bug in the code :
> 
> unsigned int count;
> 
> count = simple_strtoul(buf, &endp, 0);

Are you suggesting to change the type of "count" to unsigned long?
That seems like a separate issue from the bounds checking part.

I don't see the possible 32-bit truncation here is a serious issue though.
The user could even echo "128abc" into rps_flow_cnt and simple_strtoul()
would be happy to pick 128.

- xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 23, 2011, 5:16 a.m. UTC | #6 
Le vendredi 23 décembre 2011 à 00:10 -0500, Xi Wang a écrit :
> On Dec 22, 2011, at 11:53 PM, Eric Dumazet wrote:
> > All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
> > have spotted the other bug in the code :
> > 
> > unsigned int count;
> > 
> > count = simple_strtoul(buf, &endp, 0);
> 
> Are you suggesting to change the type of "count" to unsigned long?
> That seems like a separate issue from the bounds checking part.
> 
> I don't see the possible 32-bit truncation here is a serious issue though.
> The user could even echo "128abc" into rps_flow_cnt and simple_strtoul()
> would be happy to pick 128.
> 

32 bit truncation _is_ a bound checking problem too.

Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
when writing such hard to read code.

You cannot claim to give more range to 64bit platform, yet not spotting
the 32bit truncation issue.

When fixing a bug, its always a good thing to look things around, and
try to check the whole function.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 23, 2011, 5:35 a.m. UTC | #7 
Le vendredi 23 décembre 2011 à 06:16 +0100, Eric Dumazet a écrit :

> 32 bit truncation _is_ a bound checking problem too.
> 
> Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
> when writing such hard to read code.
> 
> You cannot claim to give more range to 64bit platform, yet not spotting
> the 32bit truncation issue.
> 
> When fixing a bug, its always a good thing to look things around, and
> try to check the whole function.
> 
> 

By the way, the theorical limit on number of flows on 64bit platform is
2^32  (rxhash being an u32)

Not sure spending 32GB per table would be wise for typical machines :)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang Dec. 23, 2011, 5:49 a.m. UTC | #8 
On Dec 23, 2011, at 12:16 AM, Eric Dumazet wrote:
> 32 bit truncation _is_ a bound checking problem too.
> 
> Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
> when writing such hard to read code.
> 
> You cannot claim to give more range to 64bit platform, yet not spotting
> the 32bit truncation issue.
> 
> When fixing a bug, its always a good thing to look things around, and
> try to check the whole function.

Okay, seems like we are talking about two different issues here.

1) the truncation of "count" on 64-bit platform.

I don't mind if the user shoots himself in the foot by setting a
large or invalid rps_flow_cnt, as long as the kernel is not affected.
I do agree with you that the kernel could be more friendly and
reject the value rather than picking up part of it.  To be nicer
the kernel may even want to reject something like "123abc" that
simple_strtoul() would accept.  I am happy to see another patch
fixing that.

2) the bounds checking of the vmalloc() size on 32-bit platform.

I agree that the two checks are ugly. ;-)  Do you want to use INT_MAX
in both checks, or do you have something more elegant in mind?

- xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang Dec. 23, 2011, 5:56 a.m. UTC | #9 
On Dec 23, 2011, at 12:35 AM, Eric Dumazet wrote:
> By the way, the theorical limit on number of flows on 64bit platform is
> 2^32  (rxhash being an u32)
> 
> Not sure spending 32GB per table would be wise for typical machines :)

True, a large rps_flow_cnt is not that useful. ;-)

Anyway, my point is that if the patch looks confusing to you, then
it is probably not good.  I am happy to see a more elegant fix.

- xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index c71c434..385aefe 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -665,11 +665,14 @@  static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 	if (count) {
 		int i;
 
-		if (count > 1<<30) {
+		if (count > INT_MAX)
+			return -EINVAL;
+		count = roundup_pow_of_two(count);
+		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
+				/ sizeof(struct rps_dev_flow)) {
 			/* Enforce a limit to prevent overflow */
 			return -EINVAL;
 		}
-		count = roundup_pow_of_two(count);
 		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
 		if (!table)
 			return -ENOMEM;