Re: Promiscuity counter underflow

[Stephen Hemminger <shemminger@xxxxxxxxxx>]

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) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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)


-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-net" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html