Hello Jim,
Thanks once again for the review. Unfortunately I could not get the
webrev containing just your changes. Before I could generate the webrev
I had done hg commit and hg recommit. So the webrev posted below are
relative to my parent workspace. Am sorry about that. The changes are
mainly in
(a) tcp_sig.c (made the list static, et al)
(b) tcp.c (changed dtrace probes to include tcp_t)
(c) ip_stack.h and tcp.h (used forward declaration to avoid void * for
tcp signature keys).
Webrev:
http://cr.opensolaris.org/~girishmg/tcpsig_latest/
Please see in-line for my responses.
thanks
~Girish
James Carlson wrote:
Girish Moodalbail writes:
http://cr.opensolaris.org/~girishmg/tcpsig_jim_changes/
New global issue: you need to convert to Mercurial. Putbacks by
Teamware are no longer possible.
Now it's mercurial. I was in the process of transition :-)
New issues:
tcp_sig.c:
86,390: thanks for using the common list structure (it makes
debugging easier), but why is this a pointer?
Not keeping it as a pointer any more. It's just a structure.
410: this doesn't work. You can't do list_next() on a pointer
to a structure that you just freed (at line 414). Here's
alternative code that will work:
while ((tsa = list_head(tdb->tdb_sa_list)) != NULL) {
list_remove(tdb->tdb_sa_list, tsa);
kmem_free(tsa->tsa_key.sak_key, tsa->tsa_key.sak_keylen);
kmem_free(tsa, sizeof (tcp_sig_sa_t));
}
Or you can use a 'tsanext' value if you prefer:
for (tsa = list_head(tdb->tdb_sa_list); tsa != NULL;
tsa = tsanext) {
tsanext = list_next(tdb->tdb_sa_list, tssa);
list_remove(tdb->tdb_sa_list, tsa);
kmem_free(tsa->tsa_key.sak_key, tsa->tsa_key.sak_keylen);
kmem_free(tsa, sizeof (tcp_sig_sa_t));
}
Ooops! oversight on my part. I have used the *while* loop suggested above.
523,525: the casts aren't really needed, and could be postively
removed with the fixes suggested below for tcp.h.
have used forward declaration to keep it simple and removed the casts
535,536: cstyle requires opening brace at end of previous line.
done
Follow-up on responses:
James Carlson wrote:
ipsecconf.c
839-843: shouldn't the known algorithm list returned by the kernel
include the new TCP-MD5 feature?
That would mean that the IPsec infrastructure supports MD5 for AH and
ESP also. However the only MD5 type algorithm supported by AH and ESP is
MD5-HMAC. So I had to work around this issue.
Oh. Are all algorithms always supported by all modes of operation?
If so, then that seems deficient to me.
In IPsec world MD5 and HMAC-MD5 are synonymous and MD5 always means
HMAC-MD5. When you run *ipsecalgs* on solaris, we get this o/p for
AH supported algorithms.
Protocol number: 2
Algorithm number: 2
Algorithm names: hmac-md5,md5
Mechanism Name: CKM_MD5_HMAC_GENERAL
Block sizes or MAC sizes: 12
Key sizes: 128
For TCP signature MD5 is different from hmac-md5. So if we introduce
this difference
in IPsec then there will be confusion.
What about this -- the subtraction of "(char)0"? That's a curious
line of code.
It's just; *key = input[i];
True isgraph() would make it easier. So used isgraph().
You'll need to say this instead:
if (!isascii(input[i]) || !isgraph(input[i])) {
The only the isascii(3C) function is defined on the full range of
integers; the others are guaranteed to work only if isascii returns
'true.' (Pedantic, I know, but I've been burned.)
done
ip_stack.h
418: why void *?
The hash table structure which holds the TCP signature SA's are defined
inside tcp_sig.c and are opaque. So defined it to be void *.
I don't think that's a good idea. It disables type checking for the
compiler and for lint.
Instead, I suggest doing what everyone else does: use 'struct
tcp_sig_db_s *', and add something like this to shut up the warnings
if you must:
/* forward declaration of opaque structure */
struct tcp_sig_db_s;
(Simpler still is to put this structure in some common location.)
used forward declaration method.
tcp.h
613,614: could these be something other than void *?
The TCP signature SA structure are defined inside tcp_sig.c and are not
visible outside that file and hence it's void *.
As above, I don't think that's the right solution. It disables too
much of the static consistency checking.
it's no more void * now.
tcp.c
5831,13450,13461,13784,13795: this doesn't look like a good choice
to me. A remote attacker could spam the system log. I suggest
either a dtrace probe point or just nothing at all.
Am using the Dtrace probes at all the locations. Please let me know if
it's ok.
I'd add the 'tcp_t' to the arguments, but otherwise, that looks better
to me.
done.
16222-16236: repeated code; looks like something that should be a
generic ipsec function.
Created new function ipsec_tcp_signature_set() as part of spd.c.
Would it be better to return the 'ipa' pointer from that function?
We don't use the *ipa* anywhere else within the function. So made
ipsec_tcp_signature_set() return boolean.
tcp_sig.c
70: I suggest using sys/list.h rather than rolling your own linked
list implementation.
Have used it. Please check the changes for correct initialization and usage.
Looks good, except for unnecessary extra pointer dereference and
allocation.
Taken care of it.
254,266: may want to do as a single allocation request.
Would not know the complete size until later. So retained as is.
I don't think that's true. The complete size is:
sizeof (tcp_sig_sa_t) + SADB_1TO8(key->sadb_key_bits)
True. I have changed it now for single allocation.
398-416: having a per-AF socket comparison function that understands
port==0 as a wildcard would be cleaner than wiping out the port
numbers and then restoring them later just so that you can use bcmp.
Have done that. Please look at tcpsig_compare_addr() function in tcp_sig.c
Passing structures on the stack seems like a wasteful way to
accomplish this. I was trying to suggest that attempting to use bcmp
here might not be the right path.
Here's an alternative I can suggest:
static boolean_t
tcpsig_compare_addr(const struct sockaddr_storage *addr1,
const struct sockaddr_storage *addr2)
{
if (addr1->ss_family != addr2->ss_family)
return (B_FALSE);
/* Only IPv4 is supported right now */
switch (addr1->ss_family) {
case AF_INET: {
const struct sockaddr_in *sin1 =
(const struct sockaddr_in *)addr1;
const struct sockaddr_in *sin2 =
(const struct sockaddr_in *)addr2;
if (sin1->sin_port != 0 && sin1->sin_port != sin2->sin_port)
return (B_FALSE);
return (sin1->sin_addr.s_addr == sin2->sin_addr.s_addr);
}
}
return (B_FALSE);
}
Have changed accordingly, please check.
430: why does this need to be TCP_SIG_BUCKETS separate allocations?
with those many hash buckets the distribution of the keys would be well
spread. It's set to 256 for now. Are you suggesting a single bucket with
keys in them? or some smaller values of buckets.
Neither -- I'm suggesting making one allocation of TCP_SIG_BUCKETS
structures.
done.
477-489: this code seems misplaced. It's actually needed only when
we search for an SA for the first time -- at line 555. We shouldn't
need to do this work on every packet.
We also need it for the computation of MD5 for the packet (mblk_t) passed
to tcp_signature().
MD5Update(&ctx, (char *)&ip->ipha_src, sizeof (ip->ipha_src));
MD5Update(&ctx, (char *)&ip->ipha_dst, sizeof (ip->ipha_dst));
I don't understand. Those are using the 'ip' pointer, which refers to
offsets off of the b_rptr pointer in the mblk_t. That has nothing to
do with filling in the stack-allocated structures named 'src' and
'dst', and which are not used in MD5 computation.
You are Right, we don?t need to do it per packet. Have moved the logic
to line 522.
482,489: bcopy?
yes th->th_lport and th->th_fport is an uint8_t array of size 2 and I
need to
copy the value to in_port_t and hence bcopy.
I see ... it'd be better if we could get this in a tcpha_t.
usually we will be performing this bcopy only once during the entire
life of the connection
so retained as is.
712: what is this about?
when "ipseckey get tcpsig" is called from CLI,
tcpsig_delget_sa(delete=B_FALSE) is
called. This will hold a reference on the "tsa". Now we need to convert
this "tsa" into
a new SADB message and reply back to the consumer. This is done by
tcpsig_newsamsg(). Once am done with building the message I release the
hold on *tsa*.
But that hold is only released "sometimes." Other paths (such as line
647) lead to the hold not being released.
Now am releasing the reference. Further that was the only path where I
forgot
to release the reference.
Either all the paths should do this, or none should.
_______________________________________________
networking-discuss mailing list
[email protected]
