Re: Promiscuity counter underflow

[Daniel Borkmann <danborkmann@xxxxxxxxxxxxxx>]

Stephen Hemminger wrote:
> On Wed, 12 May 2010 18:28:05 +0200
> Daniel Borkmann <danborkmann@xxxxxxxxxxxxxx> wrote:
>> 2010/5/12 Daniel Borkmann <danborkmann@xxxxxxxxxxxxxx>:
>>> Well, currently it is checked whether the (unsigned) promisc counter
>>> of a netdevice touches "roof" (-> UINT_MAX), but the other way round,
>>> if it touches bottom leaves unchecked for now. So in short words if the
>>> current promisc counter value is 3 and your inc value is -4 it "overflows"
>>> and corrupts the counter value.
>> Of course, this should only happen if someone really screwed up or the
>> promisc counter got somehow broken before. Btw. dev_set_promiscuity()
>> should be called instead of the internal __dev_set_promiscuity() for
>> setting the promiscuity from some kernel module (if this makes sense
>> from doing this from within the kernel) and usually the 'inc' value is 1 or -1.
>>
>>>>> 2010/5/11 Emmanuel Roullit <emmanuel@xxxxxxxxxxxxxxx>:
>>>>>> Signed-off-by: Emmanuel Roullit <emmanuel@xxxxxxxxxxxxxxx>
>>>>>>
>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>> index f769098..f49dbde 100644
>>>>>> --- a/net/core/dev.c
>>>>>> +++ b/net/core/dev.c
>>>>>> @@ -3591,6 +3591,13 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc)
>>>>>>
>>>>>>        ASSERT_RTNL();
>>>>>>
>>>>>> +       if (inc < 0 && -inc > dev->promiscuity) {
>>>>>> +               printk(KERN_WARNING "%s: promiscuity touches bottom, "
>>>>>> +                       "set promiscuity failed, promiscuity feature "
>>>>>> +                       "of device might be broken.\n", dev->name);
>>>>>> +               return -EOVERFLOW;
>>>>>> +       }
>>>>>> +
>>>>>>        dev->flags |= IFF_PROMISC;
>>>>>>        dev->promiscuity += inc;
>>>>>>        if (dev->promiscuity == 0) {
> 
> Don't over do this. I think a simple test for:
>         WARN_ON((int)dev->promiscuity + inc < 0);
> 	dev->promiscuity += inc;
> 
> is sufficient since:
>      * can only be triggered by buggy usage from kernel code
>      * WARN gives backtrace of why this occured
>      * is non fatal to system (so WARN not BUG)

Good point, it seems reasonable for warning the user this way
since it is caused by buggy code. Furthermore, the
__dev_set_promiscuity function won't be bloated with too much
if clauses.

Attachment: signature.asc
Description: OpenPGP digital signature