arpresolve: inverted logic

[Maxime Villard]

Hi,
there appears to be a wrong logic in eco_output and token_output. arpresolve
returns a non-zero value on failure, but these functions think it returns zero.
So when arpresolve succeeds, these functions return 0 and leak the mbuf; when
arpresolve fails, these functions believe it's ok and use 'm' while it has been
freed. In short, either a memory leak or a use-after-free.

I have written a quick patch [1], which I cannot test. Could someone please
review it?

Thanks,
Maxime

[1] http://m00nbsd.net/garbage/arp/arpresolve.diff

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Maxime Villard
Hi,
there appears to be a wrong logic in eco_output and token_output. arpresolve
returns a non-zero value on failure, but these functions think it returns zero.
So when arpresolve succeeds, these functions return 0 and leak the mbuf; when
arpresolve fails, these functions believe it's ok and use 'm' while it has been
freed. In short, either a memory leak or a use-after-free.
I have written a quick patch [1], which I cannot test. Could someone please
review it?
Thanks,
Maxime
[1] http://m00nbsd.net/garbage/arp/arpresolve.diff

LGTM

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Maxime Villard]

Post by Christos Zoulas

Post by Maxime Villard
Hi,
there appears to be a wrong logic in eco_output and token_output. arpresolve
returns a non-zero value on failure, but these functions think it returns zero.
So when arpresolve succeeds, these functions return 0 and leak the mbuf; when
arpresolve fails, these functions believe it's ok and use 'm' while it has been
freed. In short, either a memory leak or a use-after-free.
I have written a quick patch [1], which I cannot test. Could someone please
review it?
Thanks,
Maxime
[1] http://m00nbsd.net/garbage/arp/arpresolve.diff

LGTM
christos

Alright, thanks, I've committed the patch.

Now; do we also agree on the fact that there is a m_put_rcvif(_rcvif, &s)
missing before this goto [1]? Without talking about m_get_rcvif that could
return NULL.

It seems to me we could also optimize a bit by not unnecessarily holding
the lock if ifp->if_type != IFT_CARP.

[1] https://nxr.netbsd.org/xref/src/sys/netinet/if_arp.c#1317

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de