netdev - offload_handle issue in ipsec offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Fri, 22 Jun 2018 13:30:00 -0700
From:   Shannon Nelson <shannon.nelson@...cle.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Boris Pismenny <borisp@...lanox.com>,
        Ilan Tayari <ilant@...lanox.com>,
        Don Skidmore <donald.c.skidmore@...el.com>
Subject: offload_handle issue in ipsec offload

Hi Steffen,

While adding the ipsec-offload API to netdevsim I ran across an issue 
with the use of x->xso.offload_handle that I think needs attention, and 
would like your opinion before I try to address it.

The offload_handle is essentially an opaque magic cookie to be used by 
the driver to help track the driver's offload information.  As such, the 
driver fills it in when an SA is setup for an offload, and then the 
offload_handle value can be used in the driver's xmit handler to quickly 
find the device specific config information for setting up each packet.

Since this is driver specific information the stack code, e.g. 
xfrm_dev_offload_ok() and validate_xmit_xfrm() and a couple of esp 
functions, really shouldn't be trying to use it.  However, since they 
are checking offload_handle for == 0 to see if it has been used, the 
drivers need to be sure they have set it to non-zero.  This requirement 
is not clear in the API description at the moment.

I ran across this because my addition to netdevsim uses offload_handle 
to store the index of the array item that has the related SA 
information.  I found in testing that the Tx offload wasn't getting 
called because the array index was 0 and so xfrm_dev_offload_ok() would 
think there was no offload and bypass the offloaded Tx.  It turns out 
that the ixgbe implementation has the same bug, but I missed this in 
earlier testing.  The Mellanox driver doesn't run into this problem 
because they are storing a struct pointer rather than an array index.

Does the stack really need to check offload_handle?  I don't think it 
does, but perhaps I'm missing something.  Here's where I found it used:
  - validate_xmit_xfrm()   - added in 3dca3f38
  - xfrm_dev_offload_ok()  - added in original patch d77e38e6
  - esp4_gso_segment()     - added in 3dca3f38
  - esp_xmit()             - added in fca11ebd
  - esp6_gso_segment()     - added in 3dca3f38
  - esp6_xmit()            - added in 3dca3f38

I think in all cases it is unnecessary and could be pulled out.

If these checks need to be left in, then any client drivers need to be 
sure this is a non-zero value, possibly by adding a VALID bit to their 
opaque value.  The ixgbe and netdevsim implementations would be fine 
with using a high bit as a VALID flag, but that might not play well with 
drivers like Mellanox that store a struct address.

So, after all that... shall we remove the offload_handle references in 
the stack code?

sln

-- 
==================================================
Shannon Nelson           shannon.nelson@...cle.com
Parents can't afford to be squeamish