Page MenuHomeFreeBSD

Fix a race in udp6_output
ClosedPublic

Authored by tuexen on Jul 12 2019, 7:59 AM.

Details

Summary

r348494 fixes a race in udp_output(). The same race exists in udp_output6() and was discovered by syzkaller. Therefore apply a similar patch to IPv6.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Jul 12 2019, 7:59 AM
ae added inline comments.Jul 12 2019, 9:37 AM
sys/netinet6/udp6_usrreq.c
757 ↗(On Diff #59678)

It seems it is not possible for sin6 become not NULL here. Maybe it would be simpler to add goto to the "else {}" branch of this condition?

ae added inline comments.Jul 12 2019, 9:40 AM
sys/netinet6/udp6_usrreq.c
757 ↗(On Diff #59678)

I mean sin6 can not become NULL

tuexen added inline comments.Jul 12 2019, 11:27 AM
sys/netinet6/udp6_usrreq.c
757 ↗(On Diff #59678)

If sin6 is NULL it will stay so, and there is no need to retry. However, if sin6 is not NULL and the inp based condition changed after getting the inp lock, we need to retry. So I think the condition is correct. Am I missing something?

ae added inline comments.Jul 12 2019, 1:46 PM
sys/netinet6/udp6_usrreq.c
757 ↗(On Diff #59678)

Yes, I'm agree that condition is correct, I'm just a bit confused, why we need to make the check three times.
Also, it seems that bound socket can not have inp_lport = 0, so may be it is enough to only check inp_lport?

markj accepted this revision.Jul 12 2019, 4:02 PM

This matches the IPv4 change, so I'm fine with it going in. I agree that having a goto to the read lock case would make a bit more sense.

This revision is now accepted and ready to land.Jul 12 2019, 4:02 PM
tuexen added inline comments.Jul 12 2019, 8:26 PM
sys/netinet6/udp6_usrreq.c
757 ↗(On Diff #59678)

I agree with you that the code might not be optimal. But the intend of this fix is to fix a bug FreeBSD had in the IPv4 and IPv6 code path and that was fixed in the IPv4 code path, but not in the IPv6 code path. So I would like to fix it here in a consistent way.

Improving the code could be done in a separate commit, doing similar changes to the IPv4 and IV6 code to keep it as consistent as possible.
ae@: OK?

bz accepted this revision.Jul 13 2019, 10:01 AM

Thanks, was still on my TODO. Always happy if someone does the work for me :-)

This revision was automatically updated to reflect the committed changes.