gre_parse_header stops parsing when csum_err is encountered, which means
tpi->key is undefined and ip_tunnel_lookup will return NULL improperly.
This patch introduce a NULL pointer as csum_err parameter. Even when
csum_err is encountered, it won't return error and continue parsing gre
header as expected.
Fixes: 9f57c67c379d ("gre: Remove support for sharing GRE protocol hook.")
Reported-by: Jiri Benc <[email protected]>
Signed-off-by: Haishuang Yan <[email protected]>
---
net/ipv4/gre_demux.c | 2 +-
net/ipv4/ip_gre.c | 9 +++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
index b798862..679a527 100644
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -86,7 +86,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
options = (__be32 *)(greh + 1);
if (greh->flags & GRE_CSUM) {
- if (skb_checksum_simple_validate(skb)) {
+ if (csum_err && skb_checksum_simple_validate(skb)) {
*csum_err = true;
return -EINVAL;
}
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 8cce0e9..c3385a8 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -232,13 +232,10 @@ static void gre_err(struct sk_buff *skb, u32 info)
const int type = icmp_hdr(skb)->type;
const int code = icmp_hdr(skb)->code;
struct tnl_ptk_info tpi;
- bool csum_err = false;
- if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP),
- iph->ihl * 4) < 0) {
- if (!csum_err) /* ignore csum errors. */
- return;
- }
+ if (gre_parse_header(skb, &tpi, NULL, htons(ETH_P_IP),
+ iph->ihl * 4) < 0)
+ return;
if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
ipv4_update_pmtu(skb, dev_net(skb->dev), info,
--
1.8.3.1
Same as ip_gre, use gre_parse_header to parse gre header in gre error
handler code.
Signed-off-by: Haishuang Yan <[email protected]>
---
net/ipv6/ip6_gre.c | 26 ++++----------------------
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index e493b04..515adbd 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -427,35 +427,17 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
u8 type, u8 code, int offset, __be32 info)
{
struct net *net = dev_net(skb->dev);
- const struct gre_base_hdr *greh;
const struct ipv6hdr *ipv6h;
- int grehlen = sizeof(*greh);
+ struct tnl_ptk_info tpi;
struct ip6_tnl *t;
- int key_off = 0;
- __be16 flags;
- __be32 key;
- if (!pskb_may_pull(skb, offset + grehlen))
- return;
- greh = (const struct gre_base_hdr *)(skb->data + offset);
- flags = greh->flags;
- if (flags & (GRE_VERSION | GRE_ROUTING))
+ if (gre_parse_header(skb, &tpi, NULL, htons(ETH_P_IPV6),
+ offset) < 0)
return;
- if (flags & GRE_CSUM)
- grehlen += 4;
- if (flags & GRE_KEY) {
- key_off = grehlen + offset;
- grehlen += 4;
- }
- if (!pskb_may_pull(skb, offset + grehlen))
- return;
ipv6h = (const struct ipv6hdr *)skb->data;
- greh = (const struct gre_base_hdr *)(skb->data + offset);
- key = key_off ? *(__be32 *)(skb->data + key_off) : 0;
-
t = ip6gre_tunnel_lookup(skb->dev, &ipv6h->daddr, &ipv6h->saddr,
- key, greh->protocol);
+ tpi.key, tpi.proto);
if (!t)
return;
--
1.8.3.1
From: Haishuang Yan <[email protected]>
Date: Wed, 12 Sep 2018 17:21:21 +0800
> @@ -86,7 +86,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>
> options = (__be32 *)(greh + 1);
> if (greh->flags & GRE_CSUM) {
> - if (skb_checksum_simple_validate(skb)) {
> + if (csum_err && skb_checksum_simple_validate(skb)) {
> *csum_err = true;
> return -EINVAL;
> }
You want to ignore csum errors, but you do not want to elide the side
effects of the skb_checksum_simple_validate() call which are to set
skb->csum_valid and skb->csum.
Therefore, the skb_checksum_simple_validate() call still needs to be
performed. We just wont return -EINVAL in the NULL csum_err case.
> On 2018??9??14??, at ????1:58, David Miller <[email protected]> wrote:
>
> From: Haishuang Yan <[email protected]>
> Date: Wed, 12 Sep 2018 17:21:21 +0800
>
>> @@ -86,7 +86,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>>
>> options = (__be32 *)(greh + 1);
>> if (greh->flags & GRE_CSUM) {
>> - if (skb_checksum_simple_validate(skb)) {
>> + if (csum_err && skb_checksum_simple_validate(skb)) {
>> *csum_err = true;
>> return -EINVAL;
>> }
>
> You want to ignore csum errors, but you do not want to elide the side
> effects of the skb_checksum_simple_validate() call which are to set
> skb->csum_valid and skb->csum.
>
> Therefore, the skb_checksum_simple_validate() call still needs to be
> performed. We just wont return -EINVAL in the NULL csum_err case.
>
>
How about doing like this:
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -86,13 +86,14 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
options = (__be32 *)(greh + 1);
if (greh->flags & GRE_CSUM) {
- if (skb_checksum_simple_validate(skb)) {
+ if (!skb_checksum_simple_validate(skb)) {
+ skb_checksum_try_convert(skb, IPPROTO_GRE, 0,
+ null_compute_pseudo);
+ } else if (csum_err) {
*csum_err = true;
return -EINVAL;
}
- skb_checksum_try_convert(skb, IPPROTO_GRE, 0,
- null_compute_pseudo);
Thanks for reviewing.
On 13/09/18 18:58, David Miller wrote:
> From: Haishuang Yan <[email protected]>
> Date: Wed, 12 Sep 2018 17:21:21 +0800
>
>> @@ -86,7 +86,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>>
>> options = (__be32 *)(greh + 1);
>> if (greh->flags & GRE_CSUM) {
>> - if (skb_checksum_simple_validate(skb)) {
>> + if (csum_err && skb_checksum_simple_validate(skb)) {
>> *csum_err = true;
>> return -EINVAL;
>> }
> You want to ignore csum errors, but you do not want to elide the side
> effects of the skb_checksum_simple_validate() call which are to set
> skb->csum_valid and skb->csum.
>
> Therefore, the skb_checksum_simple_validate() call still needs to be
> performed. We just wont return -EINVAL in the NULL csum_err case.
How about just reversing the order of the AND?
if (skb_checksum_simple_validate(skb) && csum_err) {
*csum_err = true;
return -EINVAL;
}
> On 2018??9??14??, at ????8:44, Edward Cree <[email protected]> wrote:
>
> On 13/09/18 18:58, David Miller wrote:
>> From: Haishuang Yan <[email protected]>
>> Date: Wed, 12 Sep 2018 17:21:21 +0800
>>
>>> @@ -86,7 +86,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>>>
>>> options = (__be32 *)(greh + 1);
>>> if (greh->flags & GRE_CSUM) {
>>> - if (skb_checksum_simple_validate(skb)) {
>>> + if (csum_err && skb_checksum_simple_validate(skb)) {
>>> *csum_err = true;
>>> return -EINVAL;
>>> }
>> You want to ignore csum errors, but you do not want to elide the side
>> effects of the skb_checksum_simple_validate() call which are to set
>> skb->csum_valid and skb->csum.
>>
>> Therefore, the skb_checksum_simple_validate() call still needs to be
>> performed. We just wont return -EINVAL in the NULL csum_err case.
>
> How about just reversing the order of the AND?
>
> if (skb_checksum_simple_validate(skb) && csum_err) {
> *csum_err = true;
> return -EINVAL;
> }
>
>
It looks good to me, thanks!
But skb_checksum_try_convert only need to be called after the checksum is
validated, so I suggested a better solution as following:
89 if (!skb_checksum_simple_validate(skb)) {
90 skb_checksum_try_convert(skb, IPPROTO_GRE, 0,
91 null_compute_pseudo);
92 } else if (csum_err) {
93 *csum_err = true;
94 return -EINVAL;
95 }