From 4d3ea1882af4d141273d400fdb6e8724c5033139 Mon Sep 17 00:00:00 2001 From: Sergey Nikitin Date: Fri, 27 Oct 2023 17:06:07 +0300 Subject: [PATCH 1/2] driver: fix out-of-bounds UBSAN warning Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC") in Linux-6.5 we can't use arrays of size [0] or [1] because it triggers UBSAN false-positive warnings "array-index-out-of-bounds". Use __DECLARE_FLEX_ARRAY() macro to declare flexible arrays in those cases. For arrays with size [1] use a union to keep the size of a struct containing the flexible array unchanged. Signed-off-by: Sergey Nikitin (cherry picked from commit fe3c1896db468aa36d4eae8bf665eddff4a44f1d) --- src/driver/linux_onload/cplane.c | 2 +- src/include/ci/driver/chrdev.h | 2 +- src/include/ci/driver/eftest_kernmem.h | 2 +- src/include/ci/internal/ip_shared_types.h | 26 ++++++++++++++----- src/include/ci/tools/sysdep.h | 18 +++++++++++++ src/lib/efrm/efrm_pd.c | 7 +++-- .../internal_tests/test_ftl.c | 3 +++ src/tools/onload_remote_monitor/ftl_defs.h | 2 +- .../onload_remote_monitor/orm_json_lib.c | 2 ++ 9 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/driver/linux_onload/cplane.c b/src/driver/linux_onload/cplane.c index 883f9f879..190115df4 100644 --- a/src/driver/linux_onload/cplane.c +++ b/src/driver/linux_onload/cplane.c @@ -536,7 +536,7 @@ void cp_release(struct oo_cplane_handle* cp) struct cp_message { struct list_head link; - char buf[0]; + __DECLARE_FLEX_ARRAY(char, buf); }; struct cp_message_buffer { diff --git a/src/include/ci/driver/chrdev.h b/src/include/ci/driver/chrdev.h index 3878b2940..e074e6277 100644 --- a/src/include/ci/driver/chrdev.h +++ b/src/include/ci/driver/chrdev.h @@ -15,7 +15,7 @@ struct ci_chrdev_registration { dev_t devid; int count; struct class* class; - struct cdev* cdevs[0]; + __DECLARE_FLEX_ARRAY(struct cdev*, cdevs); }; struct ci_chrdev_node_params { diff --git a/src/include/ci/driver/eftest_kernmem.h b/src/include/ci/driver/eftest_kernmem.h index 13db9a32e..a8f7c7f11 100644 --- a/src/include/ci/driver/eftest_kernmem.h +++ b/src/include/ci/driver/eftest_kernmem.h @@ -57,7 +57,7 @@ struct eftest_kmcount_req { struct eftest_kmretrieve_req { int count_in_out; /* IN: expected OUT: actual */ - struct eftest_kmem_req reqs_out[0]; + __DECLARE_FLEX_ARRAY(struct eftest_kmem_req, reqs_out); } __attribute__((packed)); #define EFTEST_KM_IOC_MAGIC ('k') diff --git a/src/include/ci/internal/ip_shared_types.h b/src/include/ci/internal/ip_shared_types.h index 82a43c371..812346bbd 100644 --- a/src/include/ci/internal/ip_shared_types.h +++ b/src/include/ci/internal/ip_shared_types.h @@ -501,9 +501,12 @@ struct ci_pkt_zc_payload { struct { ci_uint64 app_cookie CI_ALIGN(8); /* From onload_zc_iovec::app_cookie */ ef_addrspace addr_space CI_ALIGN(8); /* Address space of this data segment */ - ef_addr dma_addr[0] CI_ALIGN(8); /* Length is oo_stack_intf_max() */ + __DECLARE_FLEX_ARRAY(ef_addr, dma_addr) CI_ALIGN(8); /* Length is oo_stack_intf_max() */ } remote; - char local[1]; + union { + char padding; + __DECLARE_FLEX_ARRAY(char, local); + }; }; /* Space of length prefix_space is reserved at the end of the structure. */ @@ -539,7 +542,7 @@ struct ci_pkt_zc_header { * cached here to allow determination of free space in * the buffer without walking the payloads. */ ci_uint8 segs; /* Number of zc_payload structs following */ - struct ci_pkt_zc_payload data[0]; + __DECLARE_FLEX_ARRAY(struct ci_pkt_zc_payload, data); }; @@ -636,7 +639,15 @@ typedef struct { typedef struct { CI_ULCONST unsigned table_size_mask; - ci_netif_filter_table_entry_fast table[1]; + /* table[1] declaration is invalid in linux-6.5 and triggers UBSAN + * "array-index-out-of-bounds" warnings. Instead declare it as table[] with + * __DECLARE_FLEX_ARRAY macro. + * Use a union here (and in other similar places) to keep the same size + * of the structure to avoid any potential side effects. */ + union { + ci_netif_filter_table_entry_fast padding; + __DECLARE_FLEX_ARRAY(ci_netif_filter_table_entry_fast, table); + }; } ci_netif_filter_table; @@ -662,7 +673,10 @@ typedef struct { typedef struct { CI_ULCONST unsigned table_size_mask; - ci_ip6_netif_filter_table_entry table[1]; + union { + ci_ip6_netif_filter_table_entry padding; + __DECLARE_FLEX_ARRAY(ci_ip6_netif_filter_table_entry, table); + }; } ci_ip6_netif_filter_table; #endif @@ -888,7 +902,7 @@ typedef struct { /* Packet buffers allocated. This is [sets_n * PKTS_PER_SET]. */ CI_ULCONST ci_int32 n_pkts_allocated; - oo_pktbuf_set set[0]; + __DECLARE_FLEX_ARRAY(oo_pktbuf_set, set); } oo_pktbuf_manager; diff --git a/src/include/ci/tools/sysdep.h b/src/include/ci/tools/sysdep.h index 6504bf89a..f1f12f8af 100644 --- a/src/include/ci/tools/sysdep.h +++ b/src/include/ci/tools/sysdep.h @@ -62,6 +62,24 @@ typedef ci_int32 ci_uerr_t; /* range of OS user-mode return codes */ typedef ci_int32 ci_kerr_t; /* range of OS kernel-mode return codes */ +#ifndef __DECLARE_FLEX_ARRAY +#ifndef __cplusplus +/* linux < 5.16. */ +/* X-SPDX-Source-URL: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git */ +/* X-SPDX-Source-Tag: v5.16 */ +/* X-SPDX-Source-File: include/uapi/linux/stddef.h */ +/* X-SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* X-SPDX-Comment: Linux 5.16 got this macro to declare flexible arrays. + * Onload uses it. In order to keep compatibility with + * older kernels, place the macro here. */ +#define __DECLARE_FLEX_ARRAY(TYPE, NAME) \ + struct { \ + struct { } __empty_ ## NAME; \ + TYPE NAME[]; \ + } +/* X-SPDX-Restore: */ +#endif /* ! __cplusplus */ +#endif /* ! __DECLARE_FLEX_ARRAY */ /********************************************************************** * Compiler and processor dependencies. diff --git a/src/lib/efrm/efrm_pd.c b/src/lib/efrm/efrm_pd.c index db4ed9a11..d1a8a6ae5 100644 --- a/src/lib/efrm/efrm_pd.c +++ b/src/lib/efrm/efrm_pd.c @@ -90,7 +90,7 @@ struct efrm_pd { /* Buffer table manager. Needed iff vf==NULL. * For Huntington, we'll need separate managers for different * page orders.*/ - struct efrm_bt_manager bt_managers[0]; + __DECLARE_FLEX_ARRAY(struct efrm_bt_manager, bt_managers); /* !! DANGER !! Do not add any fields here; bt_managers must be * the last field. @@ -112,7 +112,10 @@ struct efrm_pd_owner_ids { * owner_ids on base VI ID. On ef10 all owner_ids are 0 based as they * are function relative. */ int base, n; - unsigned long used_ids[1]; + union { + unsigned long padding; + __DECLARE_FLEX_ARRAY(unsigned long, used_ids); + }; /* When allocating an owner id block enough memory is allocated to * continue the used_ids array sufficiently to contain n owner ids. */ diff --git a/src/tests/onload/onload_remote_monitor/internal_tests/test_ftl.c b/src/tests/onload/onload_remote_monitor/internal_tests/test_ftl.c index e12a4e54d..a4b3c0757 100644 --- a/src/tests/onload/onload_remote_monitor/internal_tests/test_ftl.c +++ b/src/tests/onload/onload_remote_monitor/internal_tests/test_ftl.c @@ -155,6 +155,9 @@ int main(int argc, char* argv[]) #define FTL_TFIELD_ARRAYOFINT(ctx, type, field_name, len, flag) \ CI_BUILD_ASSERT2( TYPE_TEST((v.field_name), type[len]) ); +#define FTL_TFIELD_FLEXARRAYOFSTRUCT(ctx, type, field_name, len, flag, cond) \ + CI_BUILD_ASSERT2( TYPE_TEST((v.field_name), type[]) ); + #define FTL_TFIELD_ARRAYOFSTRUCT(ctx, type, field_name, len, flag, cond) \ FTL_TFIELD_ARRAYOFINT(ctx, type, field_name, len, flag) diff --git a/src/tools/onload_remote_monitor/ftl_defs.h b/src/tools/onload_remote_monitor/ftl_defs.h index 2c7df883e..0de9dc07b 100644 --- a/src/tools/onload_remote_monitor/ftl_defs.h +++ b/src/tools/onload_remote_monitor/ftl_defs.h @@ -1222,7 +1222,7 @@ typedef struct oo_tcp_socket_stats oo_tcp_socket_stats; #define STRUCT_FILTER_TABLE(ctx) \ FTL_TSTRUCT_BEGIN(ctx, ci_netif_filter_table, ) \ FTL_TFIELD_INT(ctx, unsigned, table_size_mask, ORM_OUTPUT_STACK) \ - FTL_TFIELD_ARRAYOFSTRUCT(ctx, \ + FTL_TFIELD_FLEXARRAYOFSTRUCT(ctx, \ ci_netif_filter_table_entry_fast, table, 1, ORM_OUTPUT_STACK, 1) \ FTL_TSTRUCT_END(ctx) diff --git a/src/tools/onload_remote_monitor/orm_json_lib.c b/src/tools/onload_remote_monitor/orm_json_lib.c index 4e5c7a114..ea1fff16b 100644 --- a/src/tools/onload_remote_monitor/orm_json_lib.c +++ b/src/tools/onload_remote_monitor/orm_json_lib.c @@ -747,6 +747,8 @@ static void orm_dump_struct_ci_netif_config_opts(char* label, ci_netif_config_op } \ } +#define FTL_TFIELD_FLEXARRAYOFSTRUCT FTL_TFIELD_ARRAYOFSTRUCT + #define FTL_TFIELD_ANON_STRUCT_BEGIN(ctx, field_name, display_flags) \ if (output_flags & display_flags) { \ dump_buf_literal("\"" #field_name "\":{"); From 9ef9f19916a6bd6182db84138eecd063b17e3f58 Mon Sep 17 00:00:00 2001 From: Alexandra Kossovsky Date: Tue, 26 Sep 2023 12:59:14 +0300 Subject: [PATCH 2/2] driver/linux_onload: remove wrong sendpage assertion Onload used to assert that a memory chunk passed to sendpage handler is always within one page. It was true at the time when the assertion was added. It is not true any more. If a user runs two splice() calls: OS socket -> OS pipe -> Onload socket, then Onload's sendpage handler gets a whole memory-continuous packet as it was received by the NIC. Such a packet tend to be a part of a compound page, and it may cross the bounds of an ordinary page. The real sense of this assertion was "ensure that we are handling a continuous chunk". This is still true. But the assertion is bad for linux>=6.1 and for some updates of older kernels. OL-Redmine-Id: 12010 Co-developed-by: Sergey Nikitin Signed-off-by: Sergey Nikitin Signed-off-by: Alexandra Kossovsky Reviewed-by: Sergey Nikitin --- src/driver/linux_onload/tcp_sendpage.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/driver/linux_onload/tcp_sendpage.c b/src/driver/linux_onload/tcp_sendpage.c index 6eddf0393..fe0cc2b40 100644 --- a/src/driver/linux_onload/tcp_sendpage.c +++ b/src/driver/linux_onload/tcp_sendpage.c @@ -55,7 +55,6 @@ ssize_t linux_tcp_helper_fop_sendpage(struct file* filp, struct page* page, ci_assert(page); ci_assert_ge(offset, 0); ci_assert_gt(size, 0); - ci_assert_le(offset + size, CI_PAGE_SIZE); #ifndef MSG_SENDPAGE_NOTLAST /* "flags" is really "more". Convert it. */