[ovs-dev,2/2] datapath: fix csum updates for MPLS actions
diff mbox series

Message ID 1562685903-32115-2-git-send-email-gvrose8192@gmail.com
State New
Headers show
Series
  • [ovs-dev,1/2] compat: ip6_gre: fix possible use-after-free in ip6erspan_rcv
Related show

Commit Message

Gregory Rose July 9, 2019, 3:25 p.m. UTC
Upstream commit:
    commit 0e3183cd2a64843a95b62f8bd4a83605a4cf0615
    Author: John Hurley <john.hurley@netronome.com>
    Date:   Thu Jun 27 14:37:30 2019 +0100

    net: openvswitch: fix csum updates for MPLS actions

    Skbs may have their checksum value populated by HW. If this is a checksum
    calculated over the entire packet then the CHECKSUM_COMPLETE field is
    marked. Changes to the data pointer on the skb throughout the network
    stack still try to maintain this complete csum value if it is required
    through functions such as skb_postpush_rcsum.

    The MPLS actions in Open vSwitch modify a CHECKSUM_COMPLETE value when
    changes are made to packet data without a push or a pull. This occurs when
    the ethertype of the MAC header is changed or when MPLS lse fields are
    modified.

    The modification is carried out using the csum_partial function to get the
    csum of a buffer and add it into the larger checksum. The buffer is an
    inversion of the data to be removed followed by the new data. Because the
    csum is calculated over 16 bits and these values align with 16 bits, the
    effect is the removal of the old value from the CHECKSUM_COMPLETE and
    addition of the new value.

    However, the csum fed into the function and the outcome of the
    calculation are also inverted. This would only make sense if it was the
    new value rather than the old that was inverted in the input buffer.

    Fix the issue by removing the bit inverts in the csum_partial calculation.

    The bug was verified and the fix tested by comparing the folded value of
    the updated CHECKSUM_COMPLETE value with the folded value of a full
    software checksum calculation (reset skb->csum to 0 and run
    skb_checksum_complete(skb)). Prior to the fix the outcomes differed but
    after they produce the same result.

    Fixes: 25cd9ba0abc0 ("openvswitch: Add basic MPLS support to kernel")
    Fixes: bc7cc5999fd3 ("openvswitch: update checksum in {push,pop}_mpls")
    Signed-off-by: John Hurley <john.hurley@netronome.com>
    Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
    Reviewed-by: Simon Horman <simon.horman@netronome.com>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Fixes: ccf4378615e9 ("datapath: Add basic MPLS support to kernel")
Fixes: b51367aad315 ("datapath: update checksum in {push,pop}_mpls")
Cc: John Hurley <john.hurley@netronome.com>
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 datapath/actions.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

William Tu July 9, 2019, 7:32 p.m. UTC | #1
On Tue, Jul 9, 2019 at 8:25 AM Greg Rose <gvrose8192@gmail.com> wrote:
>
> Upstream commit:
>     commit 0e3183cd2a64843a95b62f8bd4a83605a4cf0615
>     Author: John Hurley <john.hurley@netronome.com>
>     Date:   Thu Jun 27 14:37:30 2019 +0100
>
>     net: openvswitch: fix csum updates for MPLS actions
>
>     Skbs may have their checksum value populated by HW. If this is a checksum
>     calculated over the entire packet then the CHECKSUM_COMPLETE field is
>     marked. Changes to the data pointer on the skb throughout the network
>     stack still try to maintain this complete csum value if it is required
>     through functions such as skb_postpush_rcsum.
>
>     The MPLS actions in Open vSwitch modify a CHECKSUM_COMPLETE value when
>     changes are made to packet data without a push or a pull. This occurs when
>     the ethertype of the MAC header is changed or when MPLS lse fields are
>     modified.
>
>     The modification is carried out using the csum_partial function to get the
>     csum of a buffer and add it into the larger checksum. The buffer is an
>     inversion of the data to be removed followed by the new data. Because the
>     csum is calculated over 16 bits and these values align with 16 bits, the
>     effect is the removal of the old value from the CHECKSUM_COMPLETE and
>     addition of the new value.
>
>     However, the csum fed into the function and the outcome of the
>     calculation are also inverted. This would only make sense if it was the
>     new value rather than the old that was inverted in the input buffer.
>
>     Fix the issue by removing the bit inverts in the csum_partial calculation.
>
>     The bug was verified and the fix tested by comparing the folded value of
>     the updated CHECKSUM_COMPLETE value with the folded value of a full
>     software checksum calculation (reset skb->csum to 0 and run
>     skb_checksum_complete(skb)). Prior to the fix the outcomes differed but
>     after they produce the same result.
>
>     Fixes: 25cd9ba0abc0 ("openvswitch: Add basic MPLS support to kernel")
>     Fixes: bc7cc5999fd3 ("openvswitch: update checksum in {push,pop}_mpls")
>     Signed-off-by: John Hurley <john.hurley@netronome.com>
>     Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>     Reviewed-by: Simon Horman <simon.horman@netronome.com>
>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Fixes: ccf4378615e9 ("datapath: Add basic MPLS support to kernel")
> Fixes: b51367aad315 ("datapath: update checksum in {push,pop}_mpls")
> Cc: John Hurley <john.hurley@netronome.com>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> ---

LGTM
Acked-by: William Tu <u9012063@gmail.com>
Ben Pfaff July 10, 2019, 8:04 p.m. UTC | #2
Thanks for the backports (William, thanks for the reviews).  I applied
these to master.
Gregory Rose July 10, 2019, 11:59 p.m. UTC | #3
Thanks Ben!

On 7/10/2019 1:04 PM, Ben Pfaff wrote:
> Thanks for the backports (William, thanks for the reviews).  I applied
> these to master.

Patch
diff mbox series

diff --git a/datapath/actions.c b/datapath/actions.c
index 5a1d320..a44e804 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -178,8 +178,7 @@  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
 		__be16 diff[] = { ~(hdr->h_proto), ethertype };
 
-		skb->csum = ~csum_partial((char *)diff, sizeof(diff),
-					~skb->csum);
+		skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
 	}
 
 	hdr->h_proto = ethertype;
@@ -273,8 +272,7 @@  static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
 		__be32 diff[] = { ~(stack->label_stack_entry), lse };
 
-		skb->csum = ~csum_partial((char *)diff, sizeof(diff),
-					  ~skb->csum);
+		skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
 	}
 
 	stack->label_stack_entry = lse;