[PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Stephen Smalley]

On 2/14/19 1:00 PM, Nazarov Sergey wrote:
> Hi, Paul!
> I've found the problem and testing it with some very specific custom lsm module. The test case was simple:
> standard TCP/IP client-server application, where server opens CIPSO labeled TCP socket, and client connecting
> to this socket with forbidden labels. After several connections kernel crashing with general memory protection or
> kernel cache inconsistent error.
> I think, the similar behaviour should be with selinux or smack in the same conditions. But I don't know them
> so good to reproduce situation.

For SELinux, you can use
https://github.com/SELinuxProject/selinux-testsuite

That includes testing of CIPSO, both connecting from a client with an 
authorized level and from a client with an unauthorized level.

Not sure about Smack; there were some tests in LTP but I don't know if 
they would exercise it.

> After applying patch, I haven't kernel crashes.
> But now I've made additional checks and found no response icmp packets. The ip_options_compile requires
> CAP_NET_RAW capability when CIPSO option compiling, if skb is NULL. I have no other ideas than returning to
> the early patch version with ip_options_compile modified. What do you think about that?


> 
> 14.02.2019, 00:42, "Paul Moore" <paul at paul-moore.com>:
>> On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey <s-nazarov at yandex.ru> wrote:
>>>   Since cipso_v4_error might be called from different network stack layers, we can't safely use icmp_send there.
>>>   icmp_send copies IP options with ip_option_echo, which uses IPCB to take access to IP header compiled data.
>>>   But after commit 971f10ec ("tcp: better TCP_SKB_CB layout to reduce cache line misses"), IPCB can't be used
>>>   above IP layer.
>>>   This patch fixes the problem by creating in cipso_v4_error a local copy of compiled IP options and using it with
>>>   introduced __icmp_send function. This looks some overloaded, but in quite rare error conditions only.
>>>
>>>   The original discussion is here:
>>>   https://lore.kernel.org/linux-security-module/16659801547571984@sas1-890ba5c2334a.qloud-c.yandex.net/
>>>
>>>   Signed-off-by: Sergey Nazarov <s-nazarov at yandex.ru>
>>>   ---
>>>    include/net/icmp.h | 9 ++++++++-
>>>    net/ipv4/cipso_ipv4.c | 18 ++++++++++++++++--
>>>    net/ipv4/icmp.c | 7 ++++---
>>>    3 files changed, 28 insertions(+), 6 deletions(-)
>>
>> Hi Sergey,
>>
>> Thanks for your work on finding this and putting a fix together. As
>> we discussed previously, I think this looks good, but can you describe
>> the testing you did to verify that this works correctly?
>>
>>>   diff --git a/include/net/icmp.h b/include/net/icmp.h
>>>   index 6ac3a5b..e0f709d 100644
>>>   --- a/include/net/icmp.h
>>>   +++ b/include/net/icmp.h
>>>   @@ -22,6 +22,7 @@
>>>
>>>    #include <net/inet_sock.h>
>>>    #include <net/snmp.h>
>>>   +#include <net/ip.h>
>>>
>>>    struct icmp_err {
>>>      int errno;
>>>   @@ -39,7 +40,13 @@ struct icmp_err {
>>>    struct sk_buff;
>>>    struct net;
>>>
>>>   -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
>>>   +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>>>   + const struct ip_options *opt);
>>>   +static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
>>>   +{
>>>   + __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
>>>   +}
>>>   +
>>>    int icmp_rcv(struct sk_buff *skb);
>>>    int icmp_err(struct sk_buff *skb, u32 info);
>>>    int icmp_init(void);
>>>   diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
>>>   index 777fa3b..234d12e 100644
>>>   --- a/net/ipv4/cipso_ipv4.c
>>>   +++ b/net/ipv4/cipso_ipv4.c
>>>   @@ -1735,13 +1735,27 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
>>>     */
>>>    void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
>>>    {
>>>   + unsigned char optbuf[sizeof(struct ip_options) + 40];
>>>   + struct ip_options *opt = (struct ip_options *)optbuf;
>>>   +
>>>           if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
>>>                   return;
>>>
>>>   + /*
>>>   + * We might be called above the IP layer,
>>>   + * so we can not use icmp_send and IPCB here.
>>>   + */
>>>   +
>>>   + memset(opt, 0, sizeof(struct ip_options));
>>>   + opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
>>>   + memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
>>>   + if (ip_options_compile(dev_net(skb->dev), opt, NULL))
>>>   + return;
>>>   +
>>>           if (gateway)
>>>   - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
>>>   + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
>>>           else
>>>   - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
>>>   + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
>>>    }
>>>
>>>    /**
>>>   diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>>>   index 065997f..3f24414 100644
>>>   --- a/net/ipv4/icmp.c
>>>   +++ b/net/ipv4/icmp.c
>>>   @@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
>>>     * MUST reply to only the first fragment.
>>>     */
>>>
>>>   -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
>>>   +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>>>   + const struct ip_options *opt)
>>>    {
>>>           struct iphdr *iph;
>>>           int room;
>>>   @@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
>>>                                             iph->tos;
>>>           mark = IP4_REPLY_MARK(net, skb_in->mark);
>>>
>>>   - if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
>>>   + if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in, opt))
>>>                   goto out_unlock;
>>>
>>>   @@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
>>>           local_bh_enable();
>>>    out:;
>>>    }
>>>   -EXPORT_SYMBOL(icmp_send);
>>>   +EXPORT_SYMBOL(__icmp_send);
>>>
>>>    static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
>>>   --
>>
>> --
>> paul moore
>> www.paul-moore.com