Kernel memory corruption in CIPSO labeled TCP packets processing.

Kernel memory corruption in CIPSO labeled TCP packets processing.

[Nazarov Sergey]

Hello!
Security modules (selinux, smack) use icmp_send for discarded incorrectly labeled network packets.
This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for INET stream connection, for example).
icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
After moving IP header data to the end of the struct tcp_skb_cb (since 3.18 kernel), this could lead to
kernel memory corruption when IP options copying.
This patch fix a bug, but I'm not sure, that this is a best solution. Perhaps someone more familiar with the
linux TCP/IP stack will offer a better one.
Thanks.

--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
 					  iph->tos;
 	mark = IP4_REPLY_MARK(net, skb_in->mark);
 
-	if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
+	if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
+			ip_hdr(skb_in)->protocol == IPPROTO_TCP ? &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
 		goto out_unlock;

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

[Casey Schaufler]

On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> Hello!
> Security modules (selinux, smack) use icmp_send for discarded incorrectly labeled network packets.
> This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for INET stream connection, for example).
> icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
> After moving IP header data to the end of the struct tcp_skb_cb (since 3.18 kernel), this could lead to
> kernel memory corruption when IP options copying.

Can you explain how that corruption might occur?
Do you have a test case?

> This patch fix a bug, but I'm not sure, that this is a best solution. Perhaps someone more familiar with the
> linux TCP/IP stack will offer a better one.
> Thanks.
>
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
>  					  iph->tos;
>  	mark = IP4_REPLY_MARK(net, skb_in->mark);
>  
> -	if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> +	if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> +			ip_hdr(skb_in)->protocol == IPPROTO_TCP ? &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
>  		goto out_unlock;
>
>

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

[Paul Moore]

On Tue, Jan 15, 2019 at 12:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> > Hello!
> > Security modules (selinux, smack) use icmp_send for discarded incorrectly labeled network packets.
> > This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for INET stream connection, for example).
> > icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
> > After moving IP header data to the end of the struct tcp_skb_cb (since 3.18 kernel), this could lead to
> > kernel memory corruption when IP options copying.
>
> Can you explain how that corruption might occur?
> Do you have a test case?

Thanks for pointing this out Nazarov.

Presumably we are going to hit a problem whenever icmp_send is called
from outside the IP layer in the stack.  We fixed a similar problem a
few years back with 04f81f0154e4 ("cipso: don't use IPCB() to locate
the CIPSO IP option").

I've CC'd netdev, as I'm guessing they will have some thoughts on
this, but my initial reaction is that your proposed patch isn't as
generic as it should be for code that lives in icmp_send().  I suspect
the safe thing to do would be to call ip_options_compile() again on
skb_in and build a local copy of the ip_options struct that could then
be used in the call to __ip_options_echo(); the code could either live
in icmp_send() or some new ip_options_echo() variant
(ip_options_echo_safe()?  I dunno).  Unfortunately, calling
ip_options_compile() is going to add some overhead, and may be a
non-starter for the netdev folks, but this is error path code, so it
might be acceptable.  Hopefully the netdev folks will have some
better, more clever suggestions.

> > This patch fix a bug, but I'm not sure, that this is a best solution. Perhaps someone more familiar with the
> > linux TCP/IP stack will offer a better one.
> > Thanks.
> >
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
> >                                         iph->tos;
> >       mark = IP4_REPLY_MARK(net, skb_in->mark);
> >
> > -     if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> > +     if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> > +                     ip_hdr(skb_in)->protocol == IPPROTO_TCP ? &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
> >               goto out_unlock;
> >
> >



-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Tue, Jan 15, 2019 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 15, 2019 at 12:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> > > Hello!
> > > Security modules (selinux, smack) use icmp_send for discarded incorrectly labeled network packets.
> > > This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for INET stream connection, for example).
> > > icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
> > > After moving IP header data to the end of the struct tcp_skb_cb (since 3.18 kernel), this could lead to
> > > kernel memory corruption when IP options copying.
> >
> > Can you explain how that corruption might occur?
> > Do you have a test case?
>
> Thanks for pointing this out Nazarov.
>
> Presumably we are going to hit a problem whenever icmp_send is called
> from outside the IP layer in the stack.  We fixed a similar problem a
> few years back with 04f81f0154e4 ("cipso: don't use IPCB() to locate
> the CIPSO IP option").
>
> I've CC'd netdev, as I'm guessing they will have some thoughts on
> this, but my initial reaction is that your proposed patch isn't as
> generic as it should be for code that lives in icmp_send().  I suspect
> the safe thing to do would be to call ip_options_compile() again on
> skb_in and build a local copy of the ip_options struct that could then
> be used in the call to __ip_options_echo(); the code could either live
> in icmp_send() or some new ip_options_echo() variant
> (ip_options_echo_safe()?  I dunno).  Unfortunately, calling
> ip_options_compile() is going to add some overhead, and may be a
> non-starter for the netdev folks, but this is error path code, so it
> might be acceptable.  Hopefully the netdev folks will have some
> better, more clever suggestions.

It's been a few days now with no comments from the netdev folks, so I
think it's probably best to start putting together a RFC patch for
review/comment.  Nazarov, would you like to do that?  If not, that's
okay, just let me know.

I'm still concerned about calling ip_options_compile() in icmp_send()
and I'm thinking we might be better off to add a new ip_options
parameter to icmp_send(); if the parameter is NULL we behave as we do
today, but if it is non-NULL we use the given ip_options parameter in
place of calling ip_options_echo().  With that change in place, we
would need to update cipso_v4_error() to do the extra work of calling
ip_options_compile() and __ip_options_echo().  There looks to be maybe
a dozen (or two?) existing icmp_send() callers, but it should be
pretty trivial to update them to pass NULL for the new parameter.

What do you think?

> > > This patch fix a bug, but I'm not sure, that this is a best solution. Perhaps someone more familiar with the
> > > linux TCP/IP stack will offer a better one.
> > > Thanks.
> > >
> > > --- a/net/ipv4/icmp.c
> > > +++ b/net/ipv4/icmp.c
> > > @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
> > >                                         iph->tos;
> > >       mark = IP4_REPLY_MARK(net, skb_in->mark);
> > >
> > > -     if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> > > +     if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> > > +                     ip_hdr(skb_in)->protocol == IPPROTO_TCP ? &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
> > >               goto out_unlock;
> > >
> > >

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

Hi, Paul!
I don't like this. As you said, fix any calls to icmp_send is a trivial.
But in cipso_v4_optptr, we repeating the work already done, and in cipso_v4_error
we will need to do even more again. Any code, working with IP header data on several
levels of TCP/IP stack need to do this again and again. Is looks too overloaded!
In my opinion, this is a problem of TCP/IP stack design, comes from standing
compiled IP header data in different places at different stack layers.
May be better to add some flag or pointer to struct sk_buff?
But, I think, we need netdev developers advice for this.
Regards, Sergey.

18.01.2019, 17:53, "Paul Moore" <paul@paul-moore.com>:
> On Tue, Jan 15, 2019 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
>
> It's been a few days now with no comments from the netdev folks, so I
> think it's probably best to start putting together a RFC patch for
> review/comment. Nazarov, would you like to do that? If not, that's
> okay, just let me know.
>
> I'm still concerned about calling ip_options_compile() in icmp_send()
> and I'm thinking we might be better off to add a new ip_options
> parameter to icmp_send(); if the parameter is NULL we behave as we do
> today, but if it is non-NULL we use the given ip_options parameter in
> place of calling ip_options_echo(). With that change in place, we
> would need to update cipso_v4_error() to do the extra work of calling
> ip_options_compile() and __ip_options_echo(). There looks to be maybe
> a dozen (or two?) existing icmp_send() callers, but it should be
> pretty trivial to update them to pass NULL for the new parameter.
>
> What do you think?
>
>
> --
> paul moore
> www.paul-moore.com

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

[Paul Moore]

On Fri, Jan 18, 2019 at 11:34 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> Hi, Paul!
> I don't like this. As you said, fix any calls to icmp_send is a trivial.
> But in cipso_v4_optptr, we repeating the work already done, and in cipso_v4_error
> we will need to do even more again.

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.

> Any code, working with IP header data on several
> levels of TCP/IP stack need to do this again and again. Is looks too overloaded!
> In my opinion, this is a problem of TCP/IP stack design, comes from standing
> compiled IP header data in different places at different stack layers.
> May be better to add some flag or pointer to struct sk_buff?
> But, I think, we need netdev developers advice for this.
> Regards, Sergey.
>
> 18.01.2019, 17:53, "Paul Moore" <paul@paul-moore.com>:
> > On Tue, Jan 15, 2019 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > It's been a few days now with no comments from the netdev folks, so I
> > think it's probably best to start putting together a RFC patch for
> > review/comment. Nazarov, would you like to do that? If not, that's
> > okay, just let me know.
> >
> > I'm still concerned about calling ip_options_compile() in icmp_send()
> > and I'm thinking we might be better off to add a new ip_options
> > parameter to icmp_send(); if the parameter is NULL we behave as we do
> > today, but if it is non-NULL we use the given ip_options parameter in
> > place of calling ip_options_echo(). With that change in place, we
> > would need to update cipso_v4_error() to do the extra work of calling
> > ip_options_compile() and __ip_options_echo(). There looks to be maybe
> > a dozen (or two?) existing icmp_send() callers, but it should be
> > pretty trivial to update them to pass NULL for the new parameter.
> >
> > What do you think?

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

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);
 
 
 static void icmp_socket_deliver(struct sk_buff *skb, u32 info)

---
[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;
+
 	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);
 
 /*

---

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

[Paul Moore]

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

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

[Nazarov Sergey]

22.01.2019, 19:49, "Paul Moore" <paul@paul-moore.com>:
>
> 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?

ip_options_compile calls icmp_send, if someting wrong. So, we'll go back
to trying to fix. ip_options_compile changes needed to avoid this.

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

[Paul Moore]

On Tue, Jan 22, 2019 at 12:35 PM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 22.01.2019, 19:49, "Paul Moore" <paul@paul-moore.com>:
> >
> > 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?
>
> ip_options_compile calls icmp_send, if someting wrong. So, we'll go back
> to trying to fix. ip_options_compile changes needed to avoid this.

Yes, exactly.  If you don't pass the skb it shouldn't attempt to call
icmp_send() in case of error.

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

22.01.2019, 20:48, "Paul Moore" <paul@paul-moore.com>:
>
> Yes, exactly. If you don't pass the skb it shouldn't attempt to call
> icmp_send() in case of error.
>
> --
> paul moore
> www.paul-moore.com

You are right, sorry. We can do that without ip_options_compile modification.
Simplified patch 2:
---
 net/ipv4/cipso_ipv4.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b..a4ed0a4 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);
 }
 
 /**

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

[Paul Moore]

On Thu, Jan 24, 2019 at 9:46 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 22.01.2019, 20:48, "Paul Moore" <paul@paul-moore.com>:
> >
> > Yes, exactly. If you don't pass the skb it shouldn't attempt to call
> > icmp_send() in case of error.
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> You are right, sorry. We can do that without ip_options_compile modification.
> Simplified patch 2:
> ---
>  net/ipv4/cipso_ipv4.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..a4ed0a4 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);

Hmm, I think the above calculation should take into account the actual
length of the IP options, and not just the max size (calculate it
based on iphdr->ihl).

Beyond that fix, I think it's time to put together a proper patchset
and post it to the lists for formal review/merging.

Thanks for your work on this.

> +       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);
>  }

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

25.01.2019, 19:46, "Paul Moore" <paul@paul-moore.com>:
> Hmm, I think the above calculation should take into account the actual
> length of the IP options, and not just the max size (calculate it
> based on iphdr->ihl).
>
> Beyond that fix, I think it's time to put together a proper patchset
> and post it to the lists for formal review/merging.
>
> Thanks for your work on this.
>
> --
> paul moore
> www.paul-moore.com

Where we can take actual IP options length? Sorry, I'm not so familiar with linux network stack.
And also, ip_options_compile could change IP options data (SSRR, LSRR, RR, TIMESTAMP options),
so, we can't use ip_options_compile again for these options. Am I right?

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

[Paul Moore]

On Mon, Jan 28, 2019 at 8:10 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 25.01.2019, 19:46, "Paul Moore" <paul@paul-moore.com>:
> > Hmm, I think the above calculation should take into account the actual
> > length of the IP options, and not just the max size (calculate it
> > based on iphdr->ihl).
> >
> > Beyond that fix, I think it's time to put together a proper patchset
> > and post it to the lists for formal review/merging.
> >
> > Thanks for your work on this.
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> Where we can take actual IP options length? Sorry, I'm not so familiar with linux network stack.

I'm the one who needs to apologize; you're doing it correctly.  Not
sure what I was thinking there, sorry about that.

> And also, ip_options_compile could change IP options data (SSRR, LSRR, RR, TIMESTAMP options),
> so, we can't use ip_options_compile again for these options. Am I right?

If we don't pass a skb into ip_options_compile(), meaning both "skb"
and "rt" will be NULL, then I don't believe the option data will
change.  Am I missing something?

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

29.01.2019, 01:18, "Paul Moore" <paul@paul-moore.com>:
> If we don't pass a skb into ip_options_compile(), meaning both "skb"
> and "rt" will be NULL, then I don't believe the option data will
> change. Am I missing something?
>
> --
> paul moore
> www.paul-moore.com

I mean, in cipso_v4_error we copy option data from skb before ip_options_compile call:
+       memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
But skb IP header data could be already changed by first call of ip_options_compile
when packet received.

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

[Paul Moore]

On Tue, Jan 29, 2019 at 2:23 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 29.01.2019, 01:18, "Paul Moore" <paul@paul-moore.com>:
> > If we don't pass a skb into ip_options_compile(), meaning both "skb"
> > and "rt" will be NULL, then I don't believe the option data will
> > change. Am I missing something?
>
> I mean, in cipso_v4_error we copy option data from skb before ip_options_compile call:
> +       memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
> But skb IP header data could be already changed by first call of ip_options_compile
> when packet received.

There are several cases where the stack ends up calling icmp_send()
after the skb has been through ip_options_compile(), that should be
okay.

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

30.01.2019, 01:42, "Paul Moore" <paul@paul-moore.com>:
> There are several cases where the stack ends up calling icmp_send()
> after the skb has been through ip_options_compile(), that should be
> okay.
>
> --
> paul moore
> www.paul-moore.com

In those cases precompiled ip_options struct used, without the need to reuse ip_options_compile.
I think, for error ICMP packet, we can discard all other options except CIPSO. It will be better, than
send packet, contains wrong option's data. Modified patch 2:
---
 net/ipv4/cipso_ipv4.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b..797826c 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,33 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
  */
 void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
 {
+       struct ip_options opt;
+       unsigned char *optptr;
+
        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.
+        *
+        * For the generated ICMP packet, we create a
+        * temporary ip _options structure, contains
+        * the CIPSO option only, since the other options data
+        * could be modified when the original packet receiving.
+        */
+
+       memset(&opt, 0, sizeof(struct ip_options));
+       optptr = cipso_v4_optptr(skb);
+       if (optptr) {
+               opt.optlen = optptr[1];
+               opt.cipso = optptr - skb_network_header(skb);
+       }
+
        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);
 }
 
 /**

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

[Paul Moore]

On Wed, Jan 30, 2019 at 8:11 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 30.01.2019, 01:42, "Paul Moore" <paul@paul-moore.com>:
> > There are several cases where the stack ends up calling icmp_send()
> > after the skb has been through ip_options_compile(), that should be
> > okay.
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> In those cases precompiled ip_options struct used, without the need to reuse ip_options_compile.
> I think, for error ICMP packet, we can discard all other options except CIPSO. It will be better, than
> send packet, contains wrong option's data. Modified patch 2:
> ---
>  net/ipv4/cipso_ipv4.c |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)

This isn't how the rest of the stack works, look at
ip_local_deliver_finish() for one example.  Perhaps the behavior you
are proposing is correct, but please show me where in the various RFC
specs it is defined so that I can better understand why it should work
this way.

> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..797826c 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,33 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
>   */
>  void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
>  {
> +       struct ip_options opt;
> +       unsigned char *optptr;
> +
>         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.
> +        *
> +        * For the generated ICMP packet, we create a
> +        * temporary ip _options structure, contains
> +        * the CIPSO option only, since the other options data
> +        * could be modified when the original packet receiving.
> +        */
> +
> +       memset(&opt, 0, sizeof(struct ip_options));
> +       optptr = cipso_v4_optptr(skb);
> +       if (optptr) {
> +               opt.optlen = optptr[1];
> +               opt.cipso = optptr - skb_network_header(skb);
> +       }
> +
>         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);
>  }
>
>  /**
>


-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

31.01.2019, 05:10, "Paul Moore" <paul@paul-moore.com>:
> This isn't how the rest of the stack works, look at
> ip_local_deliver_finish() for one example. Perhaps the behavior you
> are proposing is correct, but please show me where in the various RFC
> specs it is defined so that I can better understand why it should work
> this way.
> --
> paul moore
> www.paul-moore.com

Sorry, I was inattentive. ip_options_compile modifies srr option data, only if
skb is NULL. My last message could be ignored.

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

[Paul Moore]

On Thu, Jan 31, 2019 at 8:20 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> 31.01.2019, 05:10, "Paul Moore" <paul@paul-moore.com>:
> > This isn't how the rest of the stack works, look at
> > ip_local_deliver_finish() for one example. Perhaps the behavior you
> > are proposing is correct, but please show me where in the various RFC
> > specs it is defined so that I can better understand why it should work
> > this way.
> > --
> > paul moore
> > www.paul-moore.com
>
> Sorry, I was inattentive. ip_options_compile modifies srr option data, only if
> skb is NULL. My last message could be ignored.

Hi Nazarov,

Do you plan on submitting these patches as a proper patchset for
review and merging?

-- 
paul moore
www.paul-moore.com

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

[Nazarov Sergey]

Hi, Paul!
What I need to do for this?

11.02.2019, 23:37, "Paul Moore" <paul@paul-moore.com>:
> On Thu, Jan 31, 2019 at 8:20 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
>>  31.01.2019, 05:10, "Paul Moore" <paul@paul-moore.com>:
>>  > This isn't how the rest of the stack works, look at
>>  > ip_local_deliver_finish() for one example. Perhaps the behavior you
>>  > are proposing is correct, but please show me where in the various RFC
>>  > specs it is defined so that I can better understand why it should work
>>  > this way.
>>  > --
>>  > paul moore
>>  > www.paul-moore.com
>>
>>  Sorry, I was inattentive. ip_options_compile modifies srr option data, only if
>>  skb is NULL. My last message could be ignored.
>
> Hi Nazarov,
>
> Do you plan on submitting these patches as a proper patchset for
> review and merging?
>
> --
> paul moore
> www.paul-moore.com

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

[Paul Moore]

On Mon, Feb 11, 2019 at 4:21 PM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> Hi, Paul!
> What I need to do for this?

If you haven't already done so, go read
Documentation/process/submitting-patches.rst, that should guide you
through the process.  I would also suggest looking at both the git log
and the mailing list archives to see what others have done in terms of
commit descriptions, etc.

After that, if you have any questions let me know and I can help you out.

Thanks.

> 11.02.2019, 23:37, "Paul Moore" <paul@paul-moore.com>:
> > On Thu, Jan 31, 2019 at 8:20 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
> >>  31.01.2019, 05:10, "Paul Moore" <paul@paul-moore.com>:
> >>  > This isn't how the rest of the stack works, look at
> >>  > ip_local_deliver_finish() for one example. Perhaps the behavior you
> >>  > are proposing is correct, but please show me where in the various RFC
> >>  > specs it is defined so that I can better understand why it should work
> >>  > this way.
> >>  > --
> >>  > paul moore
> >>  > www.paul-moore.com
> >>
> >>  Sorry, I was inattentive. ip_options_compile modifies srr option data, only if
> >>  skb is NULL. My last message could be ignored.
> >
> > Hi Nazarov,
> >
> > Do you plan on submitting these patches as a proper patchset for
> > review and merging?

-- 
paul moore
www.paul-moore.com

[PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

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@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(-)

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)
--

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey <s-nazarov@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@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

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

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.
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@paul-moore.com>:
> On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey <s-nazarov@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@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

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" <paul@paul-moore.com>:
>> On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey <s-nazarov@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@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

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Tue, 12 Feb 2019 18:10:03 +0300

> 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@yandex.ru>

This problem is not unique to Cipso, net/atm/clip.c's error handler
has the same exact issue.

I didn't scan more of the tree, there are probably a couple more
locations as well.

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

Now the problem comes from TCP layer only. Is the IP over ATM operates over IP layer?

14.02.2019, 19:43, "David Miller" <davem@davemloft.net>:
> From: Nazarov Sergey <s-nazarov@yandex.ru>
> Date: Tue, 12 Feb 2019 18:10:03 +0300
>
>>  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@yandex.ru>
>
> This problem is not unique to Cipso, net/atm/clip.c's error handler
> has the same exact issue.
>
> I didn't scan more of the tree, there are probably a couple more
> locations as well.

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Thu, Feb 14, 2019 at 11:43 AM David Miller <davem@davemloft.net> wrote:
> From: Nazarov Sergey <s-nazarov@yandex.ru>
> Date: Tue, 12 Feb 2019 18:10:03 +0300
>
> > 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@yandex.ru>
>
> This problem is not unique to Cipso, net/atm/clip.c's error handler
> has the same exact issue.
>
> I didn't scan more of the tree, there are probably a couple more
> locations as well.

David, are you happy with Sergey's solution as a fix for this?

If so, would you prefer a respin of this patch to apply the to the
other broken callers (e.g. net/atm/clip.c), or would you rather merge
this patch and deal with the other callers in separate patches?

-- 
paul moore
www.paul-moore.com

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Paul Moore <paul@paul-moore.com>
Date: Fri, 15 Feb 2019 14:02:31 -0500

> On Thu, Feb 14, 2019 at 11:43 AM David Miller <davem@davemloft.net> wrote:
>> From: Nazarov Sergey <s-nazarov@yandex.ru>
>> Date: Tue, 12 Feb 2019 18:10:03 +0300
>>
>> > 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@yandex.ru>
>>
>> This problem is not unique to Cipso, net/atm/clip.c's error handler
>> has the same exact issue.
>>
>> I didn't scan more of the tree, there are probably a couple more
>> locations as well.
> 
> David, are you happy with Sergey's solution as a fix for this?
> 
> If so, would you prefer a respin of this patch to apply the to the
> other broken callers (e.g. net/atm/clip.c), or would you rather merge
> this patch and deal with the other callers in separate patches?

I'd like the other broken callers to be handled.

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Fri, Feb 15, 2019 at 3:00 PM David Miller <davem@davemloft.net> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Fri, 15 Feb 2019 14:02:31 -0500
>
> > On Thu, Feb 14, 2019 at 11:43 AM David Miller <davem@davemloft.net> wrote:
> >> From: Nazarov Sergey <s-nazarov@yandex.ru>
> >> Date: Tue, 12 Feb 2019 18:10:03 +0300
> >>
> >> > 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@yandex.ru>
> >>
> >> This problem is not unique to Cipso, net/atm/clip.c's error handler
> >> has the same exact issue.
> >>
> >> I didn't scan more of the tree, there are probably a couple more
> >> locations as well.
> >
> > David, are you happy with Sergey's solution as a fix for this?
> >
> > If so, would you prefer a respin of this patch to apply the to the
> > other broken callers (e.g. net/atm/clip.c), or would you rather merge
> > this patch and deal with the other callers in separate patches?
>
> I'd like the other broken callers to be handled.

Sergey, do you think you could fix the other callers too, or do you
want some help with that?

-- 
paul moore
www.paul-moore.com

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

I think, it would not be a good solution, if I will analyze all subsystems using icmp_send, because I do not have enough knowledge for this.
I propose to add a new function, for example, ismp_send_safe, something like that:

void icmp_send_safe(struct sk_buff *skb_in, int type, int code, __be32 info)
{
	unsigned char optbuf[sizeof(struct ip_options) + 40];
	struct ip_options *opt = (struct ip_options *)optbuf;

	memset(opt, 0, sizeof(struct ip_options));
	opt->optlen = ip_hdr(skb_in)->ihl*4 - sizeof(struct iphdr);
	memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb_in)[1]), opt->optlen);
	if (ip_options_compile(dev_net(skb_in->dev), opt, NULL))
		return;
	__icmp_send(skb_in, type, code, info, opt);
}

which could be used when the code works at different network stack layers.
And the subsystems developers, knowing their code well, will decide which one to use.

15.02.2019, 23:04, "Paul Moore" <paul@paul-moore.com>:
> On Fri, Feb 15, 2019 at 3:00 PM David Miller <davem@davemloft.net> wrote:
>>  From: Paul Moore <paul@paul-moore.com>
>>  Date: Fri, 15 Feb 2019 14:02:31 -0500
>>
>>  > On Thu, Feb 14, 2019 at 11:43 AM David Miller <davem@davemloft.net> wrote:
>>  >> From: Nazarov Sergey <s-nazarov@yandex.ru>
>>  >> Date: Tue, 12 Feb 2019 18:10:03 +0300
>>  >>
>>  >> > 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@yandex.ru>
>>  >>
>>  >> This problem is not unique to Cipso, net/atm/clip.c's error handler
>>  >> has the same exact issue.
>>  >>
>>  >> I didn't scan more of the tree, there are probably a couple more
>>  >> locations as well.
>>  >
>>  > David, are you happy with Sergey's solution as a fix for this?
>>  >
>>  > If so, would you prefer a respin of this patch to apply the to the
>>  > other broken callers (e.g. net/atm/clip.c), or would you rather merge
>>  > this patch and deal with the other callers in separate patches?
>>
>>  I'd like the other broken callers to be handled.
>
> Sergey, do you think you could fix the other callers too, or do you
> want some help with that?
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Mon, 18 Feb 2019 16:39:11 +0300

> I think, it would not be a good solution, if I will analyze all
> subsystems using icmp_send, because I do not have enough knowledge
> for this.  I propose to add a new function, for example,
> ismp_send_safe, something like that:

Please don't do this.

Solve the problem properly by auditing each case, there aren't a lot and
it is not too difficult to see the upcall sites.

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

I tried to analyze the cases of using icmp_send in kernel. It indirectly used by many protocols:
ARP, IP, UDP, Netfilter, IPVS, IPIP, GRE over IP, CLIP, XFRM, CIPSOv4.
Different IP tunnels and XFRM operating directly over IP layer and if using own skb->cb data,
having IP header data in front of it. CLIP uses icmp_send for packets from arp queue only.
So, If I right, only TCP layer moves IP header data and only CIPSOv4 operates on both IP and
TCP layers now. 

19.02.2019, 04:25, "David Miller" <davem@davemloft.net>:
> From: Nazarov Sergey <s-nazarov@yandex.ru>
> Date: Mon, 18 Feb 2019 16:39:11 +0300
>
>>  I think, it would not be a good solution, if I will analyze all
>>  subsystems using icmp_send, because I do not have enough knowledge
>>  for this. I propose to add a new function, for example,
>>  ismp_send_safe, something like that:
>
> Please don't do this.
>
> Solve the problem properly by auditing each case, there aren't a lot and
> it is not too difficult to see the upcall sites.

[PATCH v2 0/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

The original discussion is here:
https://lore.kernel.org/linux-security-module/16659801547571984@sas1-890ba5c2334a.qloud-c.yandex.net/

Changes in v2:
- original patch divided on 2 parts: add __icmp_send function, fix cipso_v4_error
- add __ip_option_compile not using icmp_send in case nof error
---
 include/net/icmp.h    |    9 ++++++++-
 include/net/ip.h      |    2 ++
 net/ipv4/cipso_ipv4.c |   17 +++++++++++++++--
 net/ipv4/icmp.c       |    7 ++++---
 net/ipv4/ip_options.c |   22 +++++++++++++++++-----
 5 files changed, 46 insertions(+), 11 deletions(-)

Re: [PATCH v2 0/2] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Fri, 22 Feb 2019 20:39:29 +0300

> The original discussion is here:
> https://lore.kernel.org/linux-security-module/16659801547571984@sas1-890ba5c2334a.qloud-c.yandex.net/
> 
> Changes in v2:
> - original patch divided on 2 parts: add __icmp_send function, fix cipso_v4_error
> - add __ip_option_compile not using icmp_send in case nof error

Your changes are lacking proper signoffs.

[PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

Add __icmp_send function having ip_options struct parameter

Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
---
 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);
 
 
 static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
---

Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Mon, Feb 25, 2019 at 11:24 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
>
> Add __icmp_send function having ip_options struct parameter
>
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
> ---
>  include/net/icmp.h    |    9 ++++++++-
>  net/ipv4/icmp.c       |    7 ++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Paul Moore <paul@paul-moore.com>

> 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);
>
>
>  static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
> ---
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Mon, 25 Feb 2019 19:24:15 +0300

> Add __icmp_send function having ip_options struct parameter
> 
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>

Applied with Subject line fixes up.  This commit doesn't even make
changes to cipse_v4_error().

Anyone who ACK'd this change or added their Reviewed-by did not read
this email and are just rubber stamping crap.

Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Mon, Feb 25, 2019 at 5:33 PM David Miller <davem@davemloft.net> wrote:
> From: Nazarov Sergey <s-nazarov@yandex.ru>
> Date: Mon, 25 Feb 2019 19:24:15 +0300
>
> > Add __icmp_send function having ip_options struct parameter
> >
> > Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
>
> Applied with Subject line fixes up.  This commit doesn't even make
> changes to cipse_v4_error().
>
> Anyone who ACK'd this change or added their Reviewed-by did not read
> this email and are just rubber stamping crap.

I should have checked the subject line closer that's my fault (Gmail
does interesting things to threads sometimes, and obscured the subject
line), however, I did look at the content of patch before giving it my
thumbs up.  Claiming the email wasn't read isn't correct (although you
could rightly argue I didn't read the subject line), or I'm "rubber
stamping crap" isn't correct.

-- 
paul moore
www.paul-moore.com

[PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

Extract IP options in cipso_v4_error and use __icmp_send.

Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
---
 include/net/ip.h      |    2 ++
 net/ipv4/cipso_ipv4.c |   17 +++++++++++++++--
 net/ipv4/ip_options.c |   22 +++++++++++++++++-----
 3 files changed, 34 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..eff86a7 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,26 @@ 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);
+	if (__ip_options_compile(dev_net(skb->dev), opt, skb, 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/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);
 
 /*
---

Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Paul Moore]

On Mon, Feb 25, 2019 at 11:27 AM Nazarov Sergey <s-nazarov@yandex.ru> wrote:
>
> Extract IP options in cipso_v4_error and use __icmp_send.
>
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
> ---
>  include/net/ip.h      |    2 ++
>  net/ipv4/cipso_ipv4.c |   17 +++++++++++++++--
>  net/ipv4/ip_options.c |   22 +++++++++++++++++-----
>  3 files changed, 34 insertions(+), 7 deletions(-)

Thanks Sergey.

Acked-by: Paul Moore <paul@paul-moore.com>

> 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..eff86a7 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,26 @@ 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);
> +       if (__ip_options_compile(dev_net(skb->dev), opt, skb, 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/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);
>
>  /*
> ---
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Mon, 25 Feb 2019 19:27:15 +0300

> Extract IP options in cipso_v4_error and use __icmp_send.
> 
> Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>

Applied.

Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

Add __icmp_send function having ip_options struct parameter
---
 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);
 
 
 static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
---

Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error

[Nazarov Sergey]

Extract IP options in cipso_v4_error and use __icmp_send.
---
 include/net/ip.h      |    2 ++
 net/ipv4/cipso_ipv4.c |   17 +++++++++++++++--
 net/ipv4/ip_options.c |   22 +++++++++++++++++-----
 3 files changed, 34 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..eff86a7 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,26 @@ 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);
+	if (__ip_options_compile(dev_net(skb->dev), opt, skb, 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/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);
 
 /*
---

Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error

[David Miller]

From: Nazarov Sergey <s-nazarov@yandex.ru>
Date: Fri, 22 Feb 2019 19:35:29 +0300

> I tried to analyze the cases of using icmp_send in kernel. It
> indirectly used by many protocols: ARP, IP, UDP, Netfilter, IPVS,
> IPIP, GRE over IP, CLIP, XFRM, CIPSOv4.  Different IP tunnels and
> XFRM operating directly over IP layer and if using own skb->cb data,
> having IP header data in front of it. CLIP uses icmp_send for
> packets from arp queue only.  So, If I right, only TCP layer moves
> IP header data and only CIPSOv4 operates on both IP and TCP layers
> now.

Ok.