Re: Network hang after c3f1010b30f7fc611139cfb702a8685741aa6827 with CIPSO & Smack

[Casey Schaufler <casey@schaufler-ca.com>]

On 7/20/2016 1:13 PM, Paul Moore wrote:
> On Tue, Jul 19, 2016 at 7:37 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Digging into this further I have determined that the
>> circumstances leading to this issue are somewhat complex.
>> The good news is that there seems to be a very limited
>> circumstances under which the problem manifests.
>>
>> I have a socket, and change the Smack attributes on the
>> socket (security_inode_setsecurity) before connecting to
>> a server.
> This is a minty fresh, disconnected socket, yes?
>
>> The connect succeeds. The client sends a packet,
>> also successfully. The response is received. Now here's
>> where it gets interesting. I instrumented the code to print
>> the Smack attributes on the socket both before and after
>> the Smack access check.
> I'm assuming that when you say "access check" you are talking about
> the smk_access() call in smack_socket_sock_rcv_skb(), yes?

I have been able to track this down to my careless
use of netlbl_skbuff_err(). Because the socket started
life as unlabeled, and changed to labeled, calling
netlbl_skbuff_err() resulted in multiple frees of some
netlabel data under some circumstances. I don't know
why it worked before, but the code certainly shouldn't
have been making that call. I have a patch in final test.

>
> (as a totally unrelated side note, you really went nuts on the cpp
> conditionals in there, was there a sale on #ifdefs that I missed? <g>)

I can't say that I'm happy about how that code ended up.
I hope to do a clean up in association with switching away
from CIPSO to secmark for local access controls. That's something
I have to do for Extreme Security Module Stacking.

>
>> Before the check is made the Smack
>> data reflects the initial values from when the socket was
>> created. After the check, they reflect the explicit change
>> made earlier.
> It has been too long since I looked at how Smack handled network
> packets, I assume this is not the intended behavior?
>
>> The check reports failure based on the initial
>> values. As a result, an attempt to notify the caller that
>> the action failed is made (netlbl_skbuff_err) which results
>> in a call to icmp_send that frees already freed memory.
> What memory is being double freed?  The original skb?  I don't believe
> netlbl_skbuff_err(), cipso_v4_error(), or icmp_send() frees the
> original skb ... or rather it shouldn't, perhaps I'm missing
> something.
>
> I'm not arguing, you saw what you saw, I'm just trying to understand
> and make sense of it.  Can you elaborate on what you saw, using very
> small words, and concrete descriptions (I'm much more stupider than
> everyone here so you have to make it easy for me to understand)?
>
>> If the Smack attributes in the sk_security blob are not
>> explicitly set the problem does not occur. I have the same
>> result if I change the Smack attributes within the socket
>> security blob as I do if I replace the security blob.