From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net] net: udp: fix handling of CHECKSUM_COMPLETE packets Date: Thu, 11 Oct 2018 12:20:19 -0700 Message-ID: <2730066b-27bd-9e74-1a83-b0057586d830@gmail.com> References: <1539282601-19795-1-git-send-email-stranche@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit To: Sean Tranchetti , davem@davemloft.net, netdev@vger.kernel.org Return-path: Received: from mail-pg1-f194.google.com ([209.85.215.194]:35461 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726867AbeJLCs4 (ORCPT ); Thu, 11 Oct 2018 22:48:56 -0400 Received: by mail-pg1-f194.google.com with SMTP id v133-v6so4633897pgb.2 for ; Thu, 11 Oct 2018 12:20:22 -0700 (PDT) In-Reply-To: <1539282601-19795-1-git-send-email-stranche@codeaurora.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10/11/2018 11:30 AM, Sean Tranchetti wrote: > Current handling of CHECKSUM_COMPLETE packets by the UDP stack is > incorrect for any packet that has an incorrect checksum value. > > udp4/6_csum_init() will both make a call to > __skb_checksum_validate_complete() to initialize/validate the csum > field when receiving a CHECKSUM_COMPLETE packet. When this packet > fails validation, skb->csum will be overwritten with the pseudoheader > checksum so the packet can be fully validated by software, but the > skb->ip_summed value will be left as CHECKSUM_COMPLETE so that way > the stack can later warn the user about their hardware spewing bad > checksums. Unfortunately, leaving the SKB in this state can cause > problems later on in the checksum calculation. > > Since the the packet is still marked as CHECKSUM_COMPLETE, > udp_csum_pull_header() will SUBTRACT the checksum of the UDP header > from skb->csum instead of adding it, leaving us with a garbage value > in that field. Once we try to copy the packet to userspace in the > udp4/6_recvmsg(), we'll make a call to skb_copy_and_csum_datagram_msg() > to checksum the packet data and add it in the garbage skb->csum value > to perform our final validation check. > > Since the value we're validating is not the proper checksum, it's possible > that the folded value could come out to 0, causing us not to drop the > packet. Instead, we believe that the packet was checksummed incorrectly > by hardware since skb->ip_summed is still CHECKSUM_COMPLETE, and we attempt > to warn the user with netdev_rx_csum_fault(skb->dev); > > Unfortunately, since this is the UDP path, skb->dev has been overwritten > by skb->dev_scratch and is no longer a valid pointer, so we end up > reading invalid memory. > > This patch addresses this problem in two ways: > 1) Remove the invalid netdev_rx_csum_fault(skb->dev) call from > skb_copy_and_csum_datagram_msg(). Since this is used by UDP > where skb->dev is invalid, trying to warn doesn't make sense. > > 2) Add better CHECKSUM_COMPLETE handling to udp4/6_csum_init(). > If we receive a packet that's CHECKSUM_COMPLETE that fails > verification (i.e. skb->csum_valid == 0), check who performed > the calculation. It's possible that the checksum was done in > software by the network stack earlier (such as Netfilter's > CONNTRACK module), and if that says the checksum is bad, > we can drop the packet immediately instead of waiting until > we try and copy it to userspace. Otherwise, we need to > mark the SKB as CHECKSUM_NONE, since the skb->csum field > no longer contains the full packet checksum after the > call to __skb_checksum_validate_complete(). > > Signed-off-by: Sean Tranchetti It would be nice if you could provide a Fixes: tag, so that we know what kind of backport work is needed. You could also CC the patch author, it is always a nice thing to do. If really we could emit a message using skb->dev after skb has been queued and RCU section gone, this always has been buggy, and maybe we should not even try to use skb->dev when warning is sent. Alternative would be to use dev index and perform a lookup to find the device, if device still exists. Thanks.