Dan McDonald wrote:
This bit of review only covers the XIPSEC comments in the refactor-review
webrev.
Thanks for the comments. A few followup things below.
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.
OK, I'll add a short comment.
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.
I'll leave it alone.
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.
I think EPERM would be plain wrong because it is associated with an
audited privilege check in the rest of the kernel. (This used to be the
check for uid == 0.)
We use EACCES for things like options that you can't set and using
reserved port numbers.
But the EACCES errno is very specific in the sendto/sendmsg man pages.
Thus I think EHOSTUNREACH is the least bad choice.
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.
Sure it is; we had to understand all this code to change it and the
stale comments are a pain when trying to do that ;-) I'll drop the comment.
There were some other XIPSEC in ip.c such as
/* Handle explicit drop action and bypass. */
switch (ap->ipa_act.ipa_type) {
case IPSEC_ACT_DISCARD:
case IPSEC_ACT_REJECT:
ip_drop_packet(mp, B_FALSE, ill,
DROPPER(ipss, ipds_spd_explicit), &ipss->ipsec_spd_dropper);
return (0); /* XIPSEC: errno for application? */
Should we use EHOSTUNREACH here as well?
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.
OK
* You may wish to do an early AH check, as is done in the IPPROTO_ROUTING
case.
What is the benefit of doing that when all the code will do is consider
sending a param_problem?
The IPPROTO_ROUTING case does more work such as advancing (a future not
type 0) routing header.
usr/src/uts/common/inet/ip/ip6_output.c
* Same ansewer as icmp.c.
OK
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.
OK
* Not sure about tsol_accept_raw.
I'll check.
* Am sure that we keep the inner header for iptun purposes.
OK
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.
OK. I'll add some code for this. It requires determining when a policy
comes from the global policy, so I'll send you a separate webrev for that.
usr/src/uts/common/inet/ip/ip_sadb.c
* ipl isn't always set if it's an unconnected socket.
OK
usr/src/uts/common/inet/ip/ipsecah.c
* You could replace ah_rput() with putnext.
Will do.
* 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.
So does it make sense to say
/*
* Based upon algorithm properties, and what-not, prioritize a
* proposal, based on the ordering of the ah algorithms in the
* alternatives in the policy rule or socket that was placed
* in the acquire record.
*/
* The sync-on-stack stuff has been addressed in my work on refactor-gate.
Yep.
* Thanks for catching the double-free.
usr/src/uts/common/inet/ip/ipsecesp.c
* Same replies as in ipsecah.c
OK
usr/src/uts/common/inet/ip/sadb.c
* inidle_overflow is correct in sadb_buf_pkt.
OK
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?
AFAICT onnv-gate handles ipl->ipl_in_policy by the hold on the ipl. But
refactor-gate uses connp->conn_latch_in_policy. I'll make sure we have a
hold on the latter.
* Perhaps renaming ipsec_check_global_policy is a good idea, but you
shouldn't get stuck doing it.
OK
* I have a fix for 2437.
Yep - already in my most recent bits.
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.
I know.
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. :(
OK, so I'll leave those two alone; you're at least aware of the CR.
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.
It is the sctp equivalent to a TCP SYN|ACK sent in response to a TCP
SYN. The difference is that we don't have a econnp since it doesn't get
allocated on the 1st packet in SCTP.
Thus the code that TCP uses can't be reused.
Given that onnv-gate has the same issue, should I just file a CR on this
and leave the code the way it was in onnv-gate?
If I do that than onnv-gate and refactor-new will use the global policy
for sending the init-ack; not look at either the policy on the listener
nor the actions on the received packet.
usr/src/uts/common/inet/sctp/sctp_error.c
* Same here as sctp_cookie.c.
ootb shutdown and error are equivalent to TCP RST. Thus I'm keeping the
same logic as for TCP RST.
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.
OK
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.
OK
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
