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.
New issues:
tcp_sig.c:
86,390: thanks for using the common list structure (it makes
debugging easier), but why is this a pointer?
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));
}
523,525: the casts aren't really needed, and could be postively
removed with the fixes suggested below for tcp.h.
535,536: cstyle requires opening brace at end of previous line.
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.
> > 932: what does this do?
What about this -- the subtraction of "(char)0"? That's a curious
line of code.
> > 933: this is hard to read ... would isascii() and isgraph() help?
> >
> 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.)
> > 1013: shouldn't flush work?
> >
> No not right now. It will be tracked as an RFE. The user need to use
> DELETE to delete the SA's. The Flush and Dump SADB_MSG's have a
> different codepath in the current gate and to make those things work for
> TCP Sig means duplicating lot of IPsec infrastructure. Further when
> these modules keysock, ah & esp are merged it can be easily implemented.
OK.
> > 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.)
> > 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.
> > 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.
> > 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?
> > 24062,24075: these hard-coded constants probably belong somewhere
> > else ... aren't they just TCPOPT_SIG_LEN + 2?
> >
> Yes. But we can't directly add it as the data offset field of tcp header
> is 4 bits
> and since these 4 bits are from MSB, we need to use the left shift
> operator. I
> have updated the comment.
OK ... seems unnecessarily filled with magic numbers, but the comment
helps, and it seems all the other users of this field do similar
things.
> > 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.
> > 214-217: what are these?
> >
> Questions to IPsec code-reviewers. In the passed down SADB message
> should I ensure whether certain fields are what it is supposed to be.
> Actually none of them is required. So have removed them.
OK; thanks.
> > 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)
> > 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);
}
> > 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.
> > 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.
> > 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.
> > 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.
Either all the paths should do this, or none should.
--
James Carlson, Solaris Networking <[EMAIL PROTECTED]>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
networking-discuss mailing list
[email protected]
