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]
Message-ID: <002201db1a75$9a83b420$cf8b1c60$@huawei.com>
Date: Wed, 9 Oct 2024 21:03:41 +0300
From: Gur Stavi <gur.stavi@...wei.com>
To: 'Willem de Bruijn' <willemdebruijn.kernel@...il.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <shuah@...nel.org>
Subject: RE: [PATCH net-next v02 1/2] af_packet: allow fanout_add when socket is not RUNNING

> Gur Stavi wrote:
> > >> @@ -1846,21 +1846,21 @@ static int fanout_add(struct sock *sk,
> struct fanout_args *args)
> > >>  	err = -EINVAL;
> > >>
> > >>  	spin_lock(&po->bind_lock);
> > >> -	if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
> > >> -	    match->type == type &&
> > >> +	if (match->type == type &&
> > >>  	    match->prot_hook.type == po->prot_hook.type &&
> > >>  	    match->prot_hook.dev == po->prot_hook.dev) {
> > >
> > > Remaining unaddressed issue is that the socket can now be added
> > > before being bound. See comment in v1.
> >
> > I extended the psock_fanout test with unbound fanout test.
> >
> > As far as I understand, the easiest way to verify bind is to test that
> > po->prot_hook.dev != NULL, since we are under a bind_lock anyway.
> > But perhaps a more readable and direct approach to test "bind" would be
> > to test po->ifindex != -1, as ifindex is commented as "bound device".
> > However, at the moment ifindex is not initialized to -1, I can add such
> > initialization, but perhaps I do not fully understand all the logic.
> >
> > Any preferences?
> 
> prot_hook.dev is not necessarily set if a packet socket is bound.
> It may be bound to any device. See dev_add_pack and ptype_head.
> 
> prot_hook.type, on the other hand, must be set if bound and is only
> modified with the bind_lock held too.
> 
> Well, and in packet_create. But setsockopt PACKET_FANOUT_ADD also
> succeeds in case bind() was not called explicitly first to bind to
> a specific device or change ptype.

Please clarify the last paragraph? When you say "also succeeds" do you
mean SHOULD succeed or MAY SUCCEED by mistake if "something" happens ???

Do you refer to the following scenario: socket is created with non-zero
protocol and becomes RUNNING "without bind" for all devices. In that case
it can be added to FANOUT without bind. Is that considered a bug or does
the bind requirement for fanout only apply for all-protocol (0) sockets?

What about using ifindex to detect bind? Initialize it to -1 in
packet_create and ensure that packet_do_bind, on success, sets it
to device id or 0?

psock_fanout, should probably be extended with scenarios that test
"all devices" and all/specific protocols. Any specific scenario
suggestions?



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ