Re: Kernel memory corruption in CIPSO labeled TCP packets processing.

[Paul Moore <paul@paul-moore.com>]

On Mon, Jan 21, 2019 at 12:11 PM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 18.01.2019, 20:17, "Paul Moore" <paul@paul-moore.com>:
> > Yes, this is extra overhead, but it is in an error path so I'm not
> > overly concerned about the performance impact on the given connection.
> >
> > I will admit that it isn't an ideal solution from a LSM/NetLabel
> > perspective, but historically the netdev folks have not been overly
> > receptive to changes which negatively impact the core network stack
> > regardless of how important it may be for the LSMs. If we could
> > modify icmp_send() so that it works regardless of the caller's
> > location in the stack, that would be great, I'm just not sure it would
> > be deemed acceptable. I suggested the approach I did because I think
> > that has the best chance of getting merged.
> >
> > Regardless, I would encourage you to put together a patch and post it
> > for review, I think that's the best way to get a response. If you
> > aren't able, or aren't comfortable, submitting a patch please let me
> > know.
> > --
> > paul moore
> > www.paul-moore.com
>
> May be something like that?
>
> [PATCH 1/2] Add IP options parameter to icmp_send
> ---
>  include/net/icmp.h    |    9 ++++++++-
>  net/ipv4/icmp.c       |    7 ++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> 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/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);

This looks reasonable, and I think this should be acceptable to the
netdev folks.  Although a minor heads-up that somebody might object to
icmp_send() no longer being exported.

More below...

> ---
> [PATCH 2/2] Extract IP options in cipso_v4_error
> ---
>  include/net/ip.h      |    2 ++
>  net/ipv4/cipso_ipv4.c |   11 +++++++++--
>  net/ipv4/ip_options.c |   22 +++++++++++++++++-----
>  3 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/ip.h b/include/net/ip.h
> index 8866bfc..f0e8d06 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct ip_options *dopt,
>  }
>
>  void ip_options_fragment(struct sk_buff *skb);
> +int __ip_options_compile(struct net *net, struct ip_options *opt,
> +                        struct sk_buff *skb, __be32 *info);
>  int ip_options_compile(struct net *net, struct ip_options *opt,
>                        struct sk_buff *skb);
>  int ip_options_get(struct net *net, struct ip_options_rcu **optp,
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..86599e9 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,20 @@ 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;
>
> +       opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
> +       if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
> +               return;

I would suggest adding a comment to the code above explaining that
this is necessary because we might be called above the IP layer and we
can't safely use IPCB() here.

>         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/ip_options.c b/net/ipv4/ip_options.c
> index ed194d4..32a3504 100644
> --- a/net/ipv4/ip_options.c
> +++ b/net/ipv4/ip_options.c
> @@ -251,8 +251,9 @@ static void spec_dst_fill(__be32 *spec_dst, struct sk_buff *skb)
>   * If opt == NULL, then skb->data should point to IP header.
>   */
>
> -int ip_options_compile(struct net *net,
> -                      struct ip_options *opt, struct sk_buff *skb)
> +int __ip_options_compile(struct net *net,
> +                        struct ip_options *opt, struct sk_buff *skb,
> +                        __be32 *info)
>  {
>         __be32 spec_dst = htonl(INADDR_ANY);
>         unsigned char *pp_ptr = NULL;
> @@ -468,11 +469,22 @@ int ip_options_compile(struct net *net,
>                 return 0;
>
>  error:
> -       if (skb) {
> -               icmp_send(skb, ICMP_PARAMETERPROB, 0, htonl((pp_ptr-iph)<<24));
> -       }
> +       if (info)
> +               *info = htonl((pp_ptr-iph)<<24);
>         return -EINVAL;
>  }
> +
> +int ip_options_compile(struct net *net,
> +                      struct ip_options *opt, struct sk_buff *skb)
> +{
> +       int ret;
> +       __be32 info;
> +
> +       ret = __ip_options_compile(net, opt, skb, &info);
> +       if (ret != 0 && skb)
> +               icmp_send(skb, ICMP_PARAMETERPROB, 0, info);
> +       return ret;
> +}
>  EXPORT_SYMBOL(ip_options_compile);

Granted I'm looking at this rather quickly, so I may be missing
something, but why the changes to ip_options_compile()?  Couldn't you
simply set opt->data manually (set the ptr) in cipso_v4_error() before
calling ip_options_compile() and arrive at the same result without
having to modify ip_options_compile()?  I suppose there is the rtable
value to worry about, but ip_options_echo() should take care of that,
yes?  No?

-- 
paul moore
www.paul-moore.com