[PATCH] division-by-zero in inet_csk_get_port

[PATCH] division-by-zero in inet_csk_get_port

[Denis V. Lunev]

This patch fixed a possible division-by-zero in inet_csk_get_port
treating situation low > high as if low == high.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Antov Arapov  <aarapov@redhat.com>

--- ./net/ipv4/inet_connection_sock.c.getport	2007-10-09 15:16:02.000000000 +0400
+++ ./net/ipv4/inet_connection_sock.c	2007-10-10 12:44:04.000000000 +0400
@@ -80,7 +80,14 @@ int inet_csk_get_port(struct inet_hashin
 		int low = sysctl_local_port_range[0];
 		int high = sysctl_local_port_range[1];
 		int remaining = (high - low) + 1;
-		int rover = net_random() % (high - low) + low;
+		int rover;
+
+		/* Treat low > high as high == low */
+		if (remaining <= 1) {
+			remaining = 1;
+			rover = low;
+		} else
+			rover = net_random() % (high - low) + low;
 
 		do {
 			head = &hashinfo->bhash[inet_bhashfn(rover, hashinfo->bhash_size)];

Re: [PATCH] division-by-zero in inet_csk_get_port

[Anton Arapov]

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


Ok, I've got it, so we have to do the same with the following:
quote from inet_hashtables.c and inet6_hashtables.c. I'll prepare the
patch.

And just a curious, does the /* Treat low > high as high == low */
idea will keep after the sysctl will be patched?

int inet_hash_connect(struct inet_timewait_death_row *death_row,
                      struct sock *sk)
{
        struct inet_hashinfo *hinfo = death_row->hashinfo;
        const unsigned short snum = inet_sk(sk)->num;
        struct inet_bind_hashbucket *head;
        struct inet_bind_bucket *tb;
        int ret;

        if (!snum) {
                int low = sysctl_local_port_range[0];
                int high = sysctl_local_port_range[1];
>                int range = high - low;
                int i;
                int port;
                static u32 hint;
                u32 offset = hint + inet_sk_port_offset(sk);
                struct hlist_node *node;
                struct inet_timewait_sock *tw = NULL;

                local_bh_disable();
                for (i = 1; i <= range; i++) {
>                        port = low + (i + offset) % range;
 

"Denis V. Lunev" <den@openvz.org> writes:
> This patch fixed a possible division-by-zero in inet_csk_get_port
> treating situation low > high as if low == high.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Antov Arapov  <aarapov@redhat.com>
>
> --- ./net/ipv4/inet_connection_sock.c.getport	2007-10-09 15:16:02.000000000 +0400
> +++ ./net/ipv4/inet_connection_sock.c	2007-10-10 12:44:04.000000000 +0400
> @@ -80,7 +80,14 @@ int inet_csk_get_port(struct inet_hashin
>  		int low = sysctl_local_port_range[0];
>  		int high = sysctl_local_port_range[1];
>  		int remaining = (high - low) + 1;
> -		int rover = net_random() % (high - low) + low;
> +		int rover;
> +
> +		/* Treat low > high as high == low */
> +		if (remaining <= 1) {
> +			remaining = 1;
> +			rover = low;
> +		} else
> +			rover = net_random() % (high - low) + low;
>  
>  		do {
>  			head = &hashinfo->bhash[inet_bhashfn(rover, hashinfo->bhash_size)];

-- 
Anton Arapov, <aarapov@redhat.com>
Kernel Development, Red Hat
GPG Key ID: 0x6FA8C812

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: [PATCH] division-by-zero in inet_csk_get_port

[David Miller]

From: Anton Arapov <aarapov@redhat.com>
Date: Wed, 10 Oct 2007 11:00:17 +0200

> Ok, I've got it, so we have to do the same with the following:
> quote from inet_hashtables.c and inet6_hashtables.c. I'll prepare the
> patch.
> 
> And just a curious, does the /* Treat low > high as high == low */
> idea will keep after the sysctl will be patched?

I'm beginning to think that we should do the sysctl validation
in this patch too, instead of duplicating this grotty check
in all of these port selection functions.

Re: [PATCH] division-by-zero in inet_csk_get_port

[Anton Arapov]

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

David Miller <davem@davemloft.net> writes:
>> Ok, I've got it, so we have to do the same with the following:
>> quote from inet_hashtables.c and inet6_hashtables.c. I'll prepare the
>> patch.
>> 
>> And just a curious, does the /* Treat low > high as high == low */
>> idea will keep after the sysctl will be patched?
>
> I'm beginning to think that we should do the sysctl validation
> in this patch too, instead of duplicating this grotty check
> in all of these port selection functions.
  Yep, that's exactly I'm talking about. I'm sure that 
  [...] % (high - low) [...] erroneous from the begining, because
in such places we want to have 1 in denominator, for the cases when we
have only one port. Because 34000 34000 in sysctl's
ip_local_port_range means 1(one) port, not 0(zero).

  So it seems to me that we have to fix mentioned denominators in
kernel/net to have 1, that will be correct logically. And do the
MAX<MIN check in sysctl code.
  From this point of view, it's best idea to have two patches: one for
the kernel/net denominators and another one for the sysctl.c's
function dointvec_minmax(). Because they can live independently. And
the patch for the kernel/net will do the work at least because we
prevent kernel trap at all.
  
  Dave, am I right?
-- 
Anton Arapov, <aarapov@redhat.com>
Kernel Development, Red Hat
GPG Key ID: 0x6FA8C812

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: [PATCH] division-by-zero in inet_csk_get_port

[David Miller]

From: Anton Arapov <aarapov@redhat.com>
Date: Wed, 10 Oct 2007 11:56:23 +0200

>   Yep, that's exactly I'm talking about. I'm sure that 
>   [...] % (high - low) [...] erroneous from the begining, because
> in such places we want to have 1 in denominator, for the cases when we
> have only one port. Because 34000 34000 in sysctl's
> ip_local_port_range means 1(one) port, not 0(zero).
> 
>   So it seems to me that we have to fix mentioned denominators in
> kernel/net to have 1, that will be correct logically. And do the
> MAX<MIN check in sysctl code.
>   From this point of view, it's best idea to have two patches: one for
> the kernel/net denominators and another one for the sysctl.c's
> function dointvec_minmax(). Because they can live independently. And
> the patch for the kernel/net will do the work at least because we
> prevent kernel trap at all.
>   
>   Dave, am I right?

Sure, two patches is fine.

Re: [PATCH] division-by-zero in inet_csk_get_port

[Anton Arapov]

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

David Miller <davem@davemloft.net> writes:
> From: Anton Arapov <aarapov@redhat.com>
> Date: Wed, 10 Oct 2007 11:56:23 +0200
>
>>   Yep, that's exactly I'm talking about. I'm sure that 
>>   [...] % (high - low) [...] erroneous from the begining, because
>> in such places we want to have 1 in denominator, for the cases when we
>> have only one port. Because 34000 34000 in sysctl's
>> ip_local_port_range means 1(one) port, not 0(zero).
>> 
>>   So it seems to me that we have to fix mentioned denominators in
>> kernel/net to have 1, that will be correct logically. And do the
>> MAX<MIN check in sysctl code.
>>   From this point of view, it's best idea to have two patches: one for
>> the kernel/net denominators and another one for the sysctl.c's
>> function dointvec_minmax(). Because they can live independently. And
>> the patch for the kernel/net will do the work at least because we
>> prevent kernel trap at all.
>>   
>>   Dave, am I right?
>
> Sure, two patches is fine.

  I have been mistaken. We can't modify sysctl code itself to do the
checks like (MAX_VAL < MIN_VAL), we have generic functions, and if we
want implement something like this we have to implement absolutely new
functionality, it's insane to do it. :)
  It seems to me, all we can is to make this check in code where the
MAX_VAL<MINVAL condition brakes logic.
  FTOH the patch that prevents do_divide_error trap is
enough... Kernel will keep working, with huge kernel panic notice in
dmesg. :)
  So, now the way suggested by Denis looks reasonable.

  What do you think?

-- 
Anton Arapov, <aarapov@redhat.com>
Kernel Development, Red Hat
GPG Key ID: 0x6FA8C812

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: [PATCH] division-by-zero in inet_csk_get_port

[Brian Haley]

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

Anton Arapov wrote:
>   So, now the way suggested by Denis looks reasonable.
> 
>   What do you think?

If that's the case then you should fix __udp_lib_get_port() the same way.

Prevent division by zero in __udp_lib_get_port() when only one 
unsecured port is available.

-Brian


Signed-off-by: Brian Haley <brian.haley@hp.com>


[-- Attachment #2: udp_port_range.patch --]
[-- Type: text/x-patch, Size: 614 bytes --]

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index ef4d901..61faa38 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -150,10 +150,11 @@ int __udp_lib_get_port(struct sock *sk, unsigned short snum,
 		int i;
 		int low = sysctl_local_port_range[0];
 		int high = sysctl_local_port_range[1];
+		int remaining = (high - low) + 1;
 		unsigned rover, best, best_size_so_far;
 
 		best_size_so_far = UINT_MAX;
-		best = rover = net_random() % (high - low) + low;
+		best = rover = net_random() % remaining + low;
 
 		/* 1st pass: look for empty (or shortest) hash chain */
 		for (i = 0; i < UDP_HTABLE_SIZE; i++) {