netdev - Re: [PATCH] net: fix /proc/net/snmp as memory corruptor

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 08 Nov 2008 21:46:36 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	David Stevens <dlstevens@...ibm.com>
CC:	Alexey Dobriyan <adobriyan@...il.com>, alan@...rguk.ukuu.org.uk,
	davem@...emloft.net, netdev@...r.kernel.org,
	netdev-owner@...r.kernel.org, Eric Sesterhenn <snakebyte@....de>
Subject: Re: [PATCH] net: fix /proc/net/snmp as memory corruptor

David Stevens a écrit :
> Oh, that was me! Thank you, Alexey!
> 
>> This should be fixed as well, or multiple threads reading /proc/net/snmp
>> could get mixed results without proper locking.
> 
> I don't believe locking is an issue here. If the values
> change between the first and second tests, being counters,
> they are still nonzero. If they are different in different
> threads, it reflects an actual change in the counter. So
> I'm not sure what you're talking about here.

If you are not sure what I am talking about, then you should probably
not use static variables at all. I found this fix quite obvious...

> 
> I don't think they should be on the stack (obviously, or
> I wouldn't have written it this way). So, FWIW, I like
> Alexey's fix, which is what the code should've been.

You apparently missed the fact that with your new code, we can have more than
16 different ICMP counters > 0.

thread 1 on CPU 1
-----------------

- fills 16 indexes in static table
- print them. good.

- fills *next* 16 indexes in static table
... preempted by some IRQ or something....

thread 2 on CPU 2
------------------

fills 16 indexes in static table, overwriting
the values that thread 1 was trying to put.
...

thread 1:

print the values of thread 2.
(it will probably prints a copy of its first line)

bang : User application missed some critical information.


> 
> For Alexey's patch:
> 
> Acked-by: David L Stevens <dlstevens@...ibm.com>

--
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