netdev - Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF

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:	Mon, 23 Jun 2008 22:44:33 +0800
From:	Wang Chen <wangchen@...fujitsu.com>
To:	Patrick McHardy <kaber@...sh.net>
CC:	Jeff Garzik <jgarzik@...ox.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	NETDEV <netdev@...r.kernel.org>, davies@...iac.ultranet.com,
	grundler@...isc-linux.org, kyle@...isc-linux.org
Subject: Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH
 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

Patrick McHardy said the following on 2008-6-23 21:47:
>>>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>>>> *rq, int cmd)
>>>>      omr &= ~OMR_PR;
>>>>      outl(omr, DE4X5_OMR);
>>>>      dev->flags &= ~IFF_PROMISC;
>>>> +    dev->promiscuity = 0;
>>>>      break;
>>> Shouldn't this be using dev_set_promiscuity().
>>>
> 
> I actually meant dev_change_flags(), sorry.
> 

dev_change_flags() can not completely change flag IFF_PROMISC like
IFF_UP, etc.
So dev_change_flags() has no big difference to dev_set_promiscuity().
I think dev_set_promiscuity() is suitable here.

>> No.
>> 1. dev_set_promiscuity do
>>   a. set/unset IFF_PROMISC
>>   b. promiscuity++/--
>>   c. audit
>>   d. dev_set_rx_mode (upload unicast and multicast list to device)
>>   Here, in ioctl, a & b is enough.
> 
> Auditing should certainly be done if promiscous mode is set.
> Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl
> handler could be changed not to care. Besides this is neither
> taking the rtnl_mutex as required nor sending notifcations
> to userspace.
> 

Agree.

>> 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
>>    replaced by dev_set_promiscuity(). Because, we don't decrease
>>    promiscuity here, but set promiscuity zero for unset IFF_PROMISC.
> 
> And that looks like a bug, the driver shouldn't disable
> promiscuity if something still requires it.
> 

It's hard to say that.
In theory, user-space can require device to disable promisc by driver's
ioctl. 
But OTOH if something else still want device to be promisc, user and
driver have no method to let them decrease the refcnt promiscuity. Because
promiscuity decrement is initiative action from upper layer, drivers don't
know who need promiscuity.

Humm, tired, go to sleep and figure out how to do after refreshing. :)

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