| Message ID | 20220504014440.3697851-1-keescook@chromium.org |
|---|---|
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote: > In preparation for run-time memcpy() bounds checking, split the nlmsg > copying for error messages (which crosses a previous unspecified flexible > array boundary) in half. Avoids the future run-time warning: > > memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg" (size 16) > > Creates an explicit flexible array at the end of nlmsghdr for the payload, > named "nlmsg_payload". There is no impact on UAPI; the sizeof(struct > nlmsghdr) does not change, but now the compiler can better reason about > where things are being copied. > > Fixed-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Link: https://lore.kernel.org/lkml/d7251d92-150b-5346-6237-52afc154bb00@rasmusvillemoes.dk > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Rich Felker <dalias@aerifal.cx> > Cc: Eric Dumazet <edumazet@google.com> > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/uapi/linux/netlink.h | 1 + > net/netlink/af_netlink.c | 5 ++++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h > index 855dffb4c1c3..47f9342d51bc 100644 > --- a/include/uapi/linux/netlink.h > +++ b/include/uapi/linux/netlink.h > @@ -47,6 +47,7 @@ struct nlmsghdr { > __u16 nlmsg_flags; /* Additional flags */ > __u32 nlmsg_seq; /* Sequence number */ > __u32 nlmsg_pid; /* Sending process port ID */ > + __u8 nlmsg_payload[];/* Contents of message */ > }; > > /* Flags values */ > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 1b5a9c2e1c29..09346aee1022 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err, > NLMSG_ERROR, payload, flags); > errmsg = nlmsg_data(rep); > errmsg->error = err; > - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); > + errmsg->msg = *nlh; > + if (payload > sizeof(*errmsg)) > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, > + nlh->nlmsg_len - sizeof(*nlh)); They have nlmsg_len()[1] for the length of the payload without the header: /** * nlmsg_len - length of message payload * @nlh: netlink message header */ static inline int nlmsg_len(const struct nlmsghdr *nlh) { return nlh->nlmsg_len - NLMSG_HDRLEN; } (would that function use some sanitization, though? what if nlmsg_len is somehow manipulated to be less than NLMSG_HDRLEN?...) Also, it seems there is at least one more instance of this same issue: diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index 16ae92054baa..d06184b94af5 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -1723,7 +1723,8 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb, nlh->nlmsg_seq, NLMSG_ERROR, payload, 0); errmsg = nlmsg_data(rep); errmsg->error = ret; - memcpy(&errmsg->msg, nlh, nlh->nlmsg_len); + errmsg->msg = *nlh; + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); cmdattr = (void *)&errmsg->msg + min_len; ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr, -- Gustavo [1] https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577
On Tue, May 03, 2022 at 06:44:29PM -0700, Kees Cook wrote: > As part of the work to perform bounds checking on all memcpy() uses, > replace the open-coded a deserialization of bytes out of memory into a > trailing flexible array by using a flex_array.h helper to perform the > allocation, bounds checking, and copying. Acked-by: Mark Brown <broonie@kernel.org>
Kees Cook <keescook@chromium.org> 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
On Thu, May 12, 2022 at 10:41:05PM +0100, David Howells wrote: > > Kees Cook <keescook@chromium.org> wrote: > > > struct afs_acl { > > - u32 size; > > - u8 data[]; > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, data); > > }; > > Oof... That's really quite unpleasant syntax. Is it not possible to have > mem_to_flex_dup() and friends work without that? You are telling them the > fields they have to fill in. Other threads discussed this too. I'm hoping to have something more flexible (pardon the pun) in v2. > [...] > or: > > ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL); > if (ret < 0) > > (or use != 0 rather than < 0) Sure, I can make the tests more explicit. The kerndoc, etc all shows it's using < 0 for errors.
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