[00/32] Introduce flexible array struct memcpy() helpers

Message

Kees Cook May 4, 2022, 1:44 a.m. UTC

Hi,

This is the next phase of memcpy() buffer bounds checking[1], which
starts by adding a new set of helpers to address common code patterns
that result in memcpy() usage that can't be easily verified by the
compiler (i.e. dynamic bounds due to flexible arrays). The runtime WARN
from memcpy has been posted before, but now there's more context around
alternatives for refactoring false positives, etc.

The core of this series is patches 2 (flex_array.h), 3 (flex_array
KUnit), and 4 (runtime memcpy WARN). Patch 1 is a fix to land before 4
(and I can send separately), and everything else are examples of what the
conversions look like for one of the helpers, mem_to_flex_dup(). These
will need to land via their respective trees, but they all depend on
patch 2, which I'm hoping to land in the coming merge window.

I'm happy to also point out that the conversions (patches 5+) are actually
a net reduction in lines of code:
 49 files changed, 154 insertions(+), 244 deletions(-)

Anyway, please let me know what you think. And apologies in advance
if this is spammy; the CC list got rather large due to the "treewide"
nature of the example conversions.

Also available here:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=flexcpy/next-20220502

-Kees

[1] https://lwn.net/Articles/864521/

Kees Cook (32):
  netlink: Avoid memcpy() across flexible array boundary
  Introduce flexible array struct memcpy() helpers
  flex_array: Add Kunit tests
  fortify: Add run-time WARN for cross-field memcpy()
  brcmfmac: Use mem_to_flex_dup() with struct brcmf_fweh_queue_item
  iwlwifi: calib: Prepare to use mem_to_flex_dup()
  iwlwifi: calib: Use mem_to_flex_dup() with struct iwl_calib_result
  iwlwifi: mvm: Use mem_to_flex_dup() with struct ieee80211_key_conf
  p54: Use mem_to_flex_dup() with struct p54_cal_database
  wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg
  nl80211: Use mem_to_flex_dup() with struct cfg80211_cqm_config
  cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies
  mac80211: Use mem_to_flex_dup() with several structs
  af_unix: Use mem_to_flex_dup() with struct unix_address
  802/garp: Use mem_to_flex_dup() with struct garp_attr
  802/mrp: Use mem_to_flex_dup() with struct mrp_attr
  net/flow_offload: Use mem_to_flex_dup() with struct flow_action_cookie
  firewire: Use __mem_to_flex_dup() with struct iso_interrupt_event
  afs: Use mem_to_flex_dup() with struct afs_acl
  ASoC: sigmadsp: Use mem_to_flex_dup() with struct sigmadsp_data
  soc: qcom: apr: Use mem_to_flex_dup() with struct apr_rx_buf
  atags_proc: Use mem_to_flex_dup() with struct buffer
  Bluetooth: Use mem_to_flex_dup() with struct
    hci_op_configure_data_path
  IB/hfi1: Use mem_to_flex_dup() for struct tid_rb_node
  Drivers: hv: utils: Use mem_to_flex_dup() with struct cn_msg
  ima: Use mem_to_flex_dup() with struct modsig
  KEYS: Use mem_to_flex_dup() with struct user_key_payload
  selinux: Use mem_to_flex_dup() with xfrm and sidtab
  xtensa: Use mem_to_flex_dup() with struct property
  usb: gadget: f_fs: Use mem_to_flex_dup() with struct ffs_buffer
  xenbus: Use mem_to_flex_dup() with struct read_buffer
  esas2r: Use __mem_to_flex() with struct atto_ioctl

 arch/arm/kernel/atags_proc.c                  |  12 +-
 arch/xtensa/platforms/xtfpga/setup.c          |   9 +-
 drivers/firewire/core-cdev.c                  |   7 +-
 drivers/hv/hv_utils_transport.c               |   7 +-
 drivers/infiniband/hw/hfi1/user_exp_rcv.c     |   7 +-
 drivers/infiniband/hw/hfi1/user_exp_rcv.h     |   4 +-
 drivers/net/wireless/ath/wcn36xx/smd.c        |   8 +-
 drivers/net/wireless/ath/wcn36xx/smd.h        |   4 +-
 .../broadcom/brcm80211/brcmfmac/fweh.c        |  11 +-
 drivers/net/wireless/intel/iwlwifi/dvm/agn.h  |   2 +-
 .../net/wireless/intel/iwlwifi/dvm/calib.c    |  23 +-
 .../net/wireless/intel/iwlwifi/dvm/ucode.c    |   8 +-
 drivers/net/wireless/intel/iwlwifi/mvm/sta.c  |   8 +-
 drivers/net/wireless/intersil/p54/eeprom.c    |   8 +-
 drivers/net/wireless/intersil/p54/p54.h       |   4 +-
 drivers/scsi/esas2r/atioctl.h                 |   1 +
 drivers/scsi/esas2r/esas2r_ioctl.c            |  11 +-
 drivers/soc/qcom/apr.c                        |  12 +-
 drivers/usb/gadget/function/f_fs.c            |  11 +-
 drivers/xen/xenbus/xenbus_dev_frontend.c      |  12 +-
 fs/afs/internal.h                             |   4 +-
 fs/afs/xattr.c                                |   7 +-
 include/keys/user-type.h                      |   4 +-
 include/linux/flex_array.h                    | 637 ++++++++++++++++++
 include/linux/fortify-string.h                |  70 +-
 include/linux/of.h                            |   3 +-
 include/linux/string.h                        |   1 +
 include/net/af_unix.h                         |  14 +-
 include/net/bluetooth/hci.h                   |   4 +-
 include/net/cfg80211.h                        |   4 +-
 include/net/flow_offload.h                    |   4 +-
 include/net/garp.h                            |   4 +-
 include/net/mac80211.h                        |   4 +-
 include/net/mrp.h                             |   4 +-
 include/uapi/linux/connector.h                |   4 +-
 include/uapi/linux/firewire-cdev.h            |   4 +-
 include/uapi/linux/netlink.h                  |   1 +
 include/uapi/linux/stddef.h                   |  14 +
 include/uapi/linux/xfrm.h                     |   4 +-
 lib/Kconfig.debug                             |  12 +-
 lib/Makefile                                  |   1 +
 lib/flex_array_kunit.c                        | 523 ++++++++++++++
 net/802/garp.c                                |   9 +-
 net/802/mrp.c                                 |   9 +-
 net/bluetooth/hci_request.c                   |   9 +-
 net/core/flow_offload.c                       |   7 +-
 net/mac80211/cfg.c                            |  22 +-
 net/mac80211/ieee80211_i.h                    |  12 +-
 net/netlink/af_netlink.c                      |   5 +-
 net/unix/af_unix.c                            |   7 +-
 net/wireless/core.h                           |   4 +-
 net/wireless/nl80211.c                        |  15 +-
 net/wireless/scan.c                           |  21 +-
 security/integrity/ima/ima_modsig.c           |  12 +-
 security/keys/user_defined.c                  |   7 +-
 security/selinux/ss/sidtab.c                  |   9 +-
 security/selinux/xfrm.c                       |   7 +-
 sound/soc/codecs/sigmadsp.c                   |  11 +-
 58 files changed, 1409 insertions(+), 253 deletions(-)
 create mode 100644 include/linux/flex_array.h
 create mode 100644 lib/flex_array_kunit.c

Comments

David Howells May 12, 2022, 9:47 p.m. UTC | #1

Kees Cook <[e-mail]> wrote:

> I'm happy to also point out that the conversions (patches 5+) are actually
> a net reduction in lines of code:
>  49 files changed, 154 insertions(+), 244 deletions(-)

That doesn't mean that it's actually code that's clearer to read.  I would say
that it's actually less clear.  In a bunch of places, you've done something
like:

-	e = kmalloc(...);
-	if (!e)
+	if (__mem_to_flex_dup(&e, ...))

The problem is that, to me at least, it looks like:

-	e = kmalloc(...);
-	if (kmalloc failed)
+	if (__mem_to_flex_dup(&e, ...) succeeded)

David