Re: [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" <p...@paul-moore.com>:
On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey <s-naza...@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/16659801547571...@sas1-890ba5c2334a.qloud-c.yandex.net/

  Signed-off-by: Sergey Nazarov <s-naza...@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