division-by-zero in inet_csk_get_port
Subsystems (4)
PATCH 1/1 division-by-zero in inet_csk_get_port
This patch fixed a possible division-by-zero in inet_csk_get_port treating situation low > high as if low == high.
net/ipv4/inet_connection_sock.c renamed from net/ipv4/inet_connection_sock.c.getport +8 -1
@@ -80,7 +80,14 @@ int inet_csk_get_port(struct inet_hashinint 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)];
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];Quoted 1 line
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++) {Quoted 1 line
port = low + (i + offset) % range;
Quoted 24 lines · Denis V. Lunev
This patch fixed a possible division-by-zero in
inet_csk_get_porttreating situation low > high as if low == high.Signed-off-by Denis V. Lunev <[email protected]>CC Antov Arapov <[email protected]>net/ipv4/inet_connection_sock.c renamed from net/ipv4/inet_connection_sock.c.getport +8
-1@@ -80,7 +80,14 @@ int inet_csk_get_port(struct inet_hashinint 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)];
Signature · 3 lines
Anton Arapov, <[email protected]> Kernel Development, Red Hat GPG Key ID: 0x6FA8C812
David Miller
maintainerFrom: Anton Arapov <[email protected]>
Date: Wed, 10 Oct 2007 11:00:17 +0200
Quoted 6 lines
Ok, I've got it, so we have to do the same with the following:
quote frominet_hashtables.candinet6_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.
Quoted 10 lines · David Miller
Quoted 6 lines
Ok, I've got it, so we have to do the same with the following:
quote frominet_hashtables.candinet6_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?
Signature · 3 lines
Anton Arapov, <[email protected]> Kernel Development, Red Hat GPG Key ID: 0x6FA8C812
David Miller
maintainerFrom: Anton Arapov <[email protected]>
Date: Wed, 10 Oct 2007 11:56:23 +0200
Quoted 15 lines
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'sip_local_port_rangemeans 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 functiondointvec_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.
Quoted 1 line
Dave, am I right?
Sure, two patches is fine.
Quoted 18 lines · David Miller
From: Anton Arapov <[email protected]>
Date: Wed, 10 Oct 2007 11:56:23 +0200Quoted 15 lines
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'sip_local_port_rangemeans 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 functiondointvec_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.
Quoted 3 lines
Quoted 1 line
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?
Signature · 3 lines
Anton Arapov, <[email protected]> Kernel Development, Red Hat GPG Key ID: 0x6FA8C812
Quoted 3 lines · Anton Arapov
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
net/ipv4/udp.c +2 -1
@@ -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++) {