| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 8 Nov 2008 09:22:19 +0300
From: Alexey Dobriyan <adobriyan@...il.com>
To: Eric Dumazet <dada1@...mosbay.com>
Cc: Eric Sesterhenn <snakebyte@....de>, davem@...emloft.net,
netdev@...r.kernel.org, alan@...rguk.ukuu.org.uk
Subject: Re: [PATCH] net: fix /proc/net/snmp as memory corruptor
On Sat, Nov 08, 2008 at 06:53:31AM +0100, Eric Dumazet wrote:
> Alexey Dobriyan a écrit :
>> On Sat, Nov 08, 2008 at 05:52:56AM +0300, Alexey Dobriyan wrote:
>>> On Sat, Nov 08, 2008 at 04:02:37AM +0300, Alexey Dobriyan wrote:
>>>> On Sat, Nov 08, 2008 at 01:22:08AM +0100, Eric Sesterhenn wrote:
>>>>> running a bunch of network related stresstests (isic, isicng,
>>>>> ...) and trying to read all files in /proc afterwards gave me two
>>>>> oopses. I was able to reproduce them on another box with
>>>>> a different config. I was able to reproduce this on 2.6.24 too,
>>>>> so this is no regression. The icmpsic is version 0.06. The
>>>>> minimal testcase to trigger this:
>>>>>
>>>>> ------------8<----------------
>>>>> #!/bin/bash
>>>>>
>>>>> icmpsic -s 127.0.0.1 -d 127.0.0.1 -p 100000
>>>>>
>>>>> find /proc/net/ | xargs cat > /dev/null
>>>>>
>>>>> cat /proc/net/ip_mr_cache
>>>>> cat /proc/net/ip_mr_vif
>>>>> ------------8<----------------
>>>>>
>>>>>
>>>>> root@...puter-desktop:~/testing# cat /proc/338/net/ip_mr_cache
>>>>>
>>>>> [ 1572.702100] BUG: unable to handle kernel NULL pointer dereferenceat 000001c1
>>>>> [ 1572.702588] IP: [<c05942c6>] ipmr_mfc_seq_show+0x26/0xf0
>>>> Reproduced.
>>> icmpsic -s 127.0.0.1 -d 127.0.0.1 -p 100000
>>> cat /proc/net/snmp # sic
>>> cat /proc/net/ip_mr_cache
>>>
>>> mfc_cache_array is full of small integers
>>>
>>> [0] = 0x1a8
>>> [1] = 0x1a9
>>>
>>> and so on.
>>
>> OK, this minimally fixes mfc_cache_array corruption.
>>
>> Someone was scared of 16 integers on stack. :^)
>
> Good spot Alexey :)
>
> This should be fixed as well, or multiple threads reading /proc/net/snmp
> could get mixed results without proper locking.
If they live in different netns, otherwise it was fine, it seems.
> ICMPMSG_MIB_MAX being 512, "int" can also be changed to "short",
> so that "short out[PERLINE]" only use 32 bytes on stack.
>
> Frankly, snmp_fold_field() results should be cached in a local array too,
> because this function can be expensive if machine has a lot of cpus.
>
> Is 128 + 32 bytes on stack considered evil ?
> Using a lock here sounds overkill, and dynamic allocation overkill too...
>
> [PATCH] net: fix /proc/net/snmp as memory corruptor
>
> icmpmsg_put() can happily corrupt kernel memory, using a static
> table and forgetting to reset an array index in a loop.
>
> Remove the static array since its not safe without proper locking.
> diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
> index 8f5a403..a631a1f 100644
> --- a/net/ipv4/proc.c
> +++ b/net/ipv4/proc.c
> @@ -237,43 +237,45 @@ static const struct snmp_mib snmp4_net_list[] = {
> SNMP_MIB_SENTINEL
> };
>
> +static void icmpmsg_put_line(struct seq_file *seq, unsigned long *vals,
> + unsigned short *type, int count)
> +{
> + int j;
> +
> + if (count) {
> + seq_printf(seq, "\nIcmpMsg:");
> + for (j = 0; j < count; ++j)
> + seq_printf(seq, " %sType%u",
> + type[j] & 0x100 ? "Out" : "In",
> + type[j] & 0xff);
> + seq_printf(seq, "\nIcmpMsg:");
> + for (j = 0; j < count; ++j)
> + seq_printf(seq, " %lu", vals[j]);
> + }
> +}
> +
> static void icmpmsg_put(struct seq_file *seq)
> {
> #define PERLINE 16
>
> - int j, i, count;
> - static int out[PERLINE];
> + int i, count;
> + unsigned short type[PERLINE];
> + unsigned long vals[PERLINE], val;
> struct net *net = seq->private;
>
> count = 0;
> for (i = 0; i < ICMPMSG_MIB_MAX; i++) {
> -
> - if (snmp_fold_field((void **) net->mib.icmpmsg_statistics, i))
> - out[count++] = i;
> - if (count < PERLINE)
> - continue;
> -
> - seq_printf(seq, "\nIcmpMsg:");
> - for (j = 0; j < PERLINE; ++j)
> - seq_printf(seq, " %sType%u", i & 0x100 ? "Out" : "In",
> - i & 0xff);
> - seq_printf(seq, "\nIcmpMsg: ");
> - for (j = 0; j < PERLINE; ++j)
> - seq_printf(seq, " %lu",
> - snmp_fold_field((void **) net->mib.icmpmsg_statistics,
> - out[j]));
> - seq_putc(seq, '\n');
> - }
> - if (count) {
> - seq_printf(seq, "\nIcmpMsg:");
> - for (j = 0; j < count; ++j)
> - seq_printf(seq, " %sType%u", out[j] & 0x100 ? "Out" :
> - "In", out[j] & 0xff);
> - seq_printf(seq, "\nIcmpMsg:");
> - for (j = 0; j < count; ++j)
> - seq_printf(seq, " %lu", snmp_fold_field((void **)
> - net->mib.icmpmsg_statistics, out[j]));
> + val = snmp_fold_field((void **) net->mib.icmpmsg_statistics, i);
> + if (val) {
> + type[count] = i;
> + vals[count++] = val;
> + }
> + if (count == PERLINE) {
> + icmpmsg_put_line(seq, vals, type, count);
> + count = 0;
> + }
> }
> + icmpmsg_put_line(seq, vals, type, count);
>
> #undef PERLINE
> }
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists