This bit of review only covers the XIPSEC comments in the refactor-review
webrev.
usr/src/uts/common/inet/ip.h
* Yes, we really need these because they are filled in during the finding of
a policy and subsquent SA, and they get placed into the ACQUIRE record.
Also, with IKEv2 coming down the stretch, we'll want the original packet's
selectors, even if the SPD rule's selectors are a superset. This is to
enable IKEv2 traffic-selector narrowing.
usr/src/uts/common/inet/sadb.h
* SA_FORM_UNIQUE_ID() is an expression often passed into a function.
It'd be hard to split this out into tunnel vs. transport.
usr/src/uts/common/inet/ip/icmp.c
* if ip_output_attach_policy() returns NULL, the packet didn't pass
IPsec policy on the outbound side (e.g. an explicit drop rule). ipdrop's
been invoked in these cases.
Perhaps EHOSTUNREACH or EPERM? The former is used by TX when these
sorts of policy failures occur.
It could also be ENOMEM if allocations were a problem.
usr/src/uts/common/inet/ip/ip.c
* FireEngine broke ipsec_override_persocket_policy and we never recovered.
Ideally we should have a way to restricting per-socket policy from
overriding the SPD, even if we don't do it by default today.
Lose the comment, but we need to fix this S10 regression at some point.
I swore I filed a bug, but I can't find one so I guess I didn't.
This isn't a problem specific to refactor.
usr/src/uts/common/inet/ip/ip6_input.c
* Yes, and you already do the right thing by passing the ira to
icmp_param_problem_nexthdr_v6(), which will do the new-world version of
ipsec_in_to_out for packet protection.
* You may wish to do an early AH check, as is done in the IPPROTO_ROUTING
case.
usr/src/uts/common/inet/ip/ip6_output.c
* Same ansewer as icmp.c.
usr/src/uts/common/inet/ip/ip_input.c
* IRAF_ESP_UDP_PORTS is for NAT-Traversal. These should be treated like ESP
packets, even though they're in UDP.
* Not sure about tsol_accept_raw.
* Am sure that we keep the inner header for iptun purposes.
usr/src/uts/common/inet/ip/ip_output.c
* If you're picking different source addresses, you'll probably need a
separate ixas UNLESS the sending socket is using per-socket policy.
usr/src/uts/common/inet/ip/ip_sadb.c
* ipl isn't always set if it's an unconnected socket.
usr/src/uts/common/inet/ip/ipsecah.c
* You could replace ah_rput() with putnext.
* You're right about the comment in ah_insert_prop() it's based on the list
of actions in the rule or socket that got slammed on to the acquire
record.
* The sync-on-stack stuff has been addressed in my work on refactor-gate.
* Thanks for catching the double-free.
usr/src/uts/common/inet/ip/ipsecesp.c
* Same replies as in ipsecah.c
usr/src/uts/common/inet/ip/sadb.c
* inidle_overflow is correct in sadb_buf_pkt.
usr/src/uts/common/inet/ip/spd.c
* I thought we took care of conn_ref before cleaning conn_latch_in_action.
Did we not fix that?
* Perhaps renaming ipsec_check_global_policy is a good idea, but you
shouldn't get stuck doing it.
* I have a fix for 2437.
usr/src/uts/common/inet/iptun/iptun.c
* 2804 -- this was what was discovered with 6886919. You need to always
call ipsec_tun_inbound() and ipsec_tun_outbound() to check global.
Unless what surrounded it changed such that 6886919 doesn't exist.
usr/src/uts/common/inet/sctp/sctp_common.c
* You probably do need to redo policy, but there's an open bug on SAs and
SCTP:
6330149 SCTP doesn't work with "sa unique".
and there are probably others.
usr/src/uts/common/inet/sctp/sctp_conn.c
* See the aforementioned IPsec + SCTP bug. :(
usr/src/uts/common/inet/sctp/sctp_cookie.c
* This logic applies IPsec? Is this an SCTP-equivalent of a reset? If so,
then yes, reflecting the policy is right. Otherwise, you only send (and
accept) the packet based on the global or per-socket policy.
usr/src/uts/common/inet/sctp/sctp_error.c
* Same here as sctp_cookie.c.
usr/src/uts/common/inet/sctp/sctp_hash.c
* Ooh, that's an onnv bug. You should pass in ipha and ip6h to
ipsec_check_inbound_policy() and lose the v4-only check.
usr/src/uts/common/inet/sctp/sctp_input.c
* Same here as with sctp_hash.
usr/src/uts/common/inet/tcp/tcp.c
* I'm not sure about just trusting the SYN. It shouldn't matter, but
the hairs on the back of my head go up if we don't do the full
inherit-policy thing.
_______________________________________________
networking-discuss mailing list
[email protected]
