| Message ID | ZQ+Nz8DfPg56pIzr@work (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eea03d18af9c44235865a4bc9bec4d780ef6cf21 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qed/red_ll2: Fix undefined behavior bug in struct qed_ll2_info | expand |
On September 23, 2023 6:15:59 PM PDT, "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: >The flexible structure (a structure that contains a flexible-array member >at the end) `qed_ll2_tx_packet` is nested within the second layer of >`struct qed_ll2_info`: > >struct qed_ll2_tx_packet { > ... > /* Flexible Array of bds_set determined by max_bds_per_packet */ > struct { > struct core_tx_bd *txq_bd; > dma_addr_t tx_frag; > u16 frag_len; > } bds_set[]; >}; > >struct qed_ll2_tx_queue { > ... > struct qed_ll2_tx_packet cur_completing_packet; >}; > >struct qed_ll2_info { > ... > struct qed_ll2_tx_queue tx_queue; > struct qed_ll2_cbs cbs; >}; Nice find! Was this located with -Wflex-array-member-not-at-end ? > [...] >Fix this by moving the declaration of `cbs` to the middle of its >containing structure `qed_ll2_info`, preventing it from being >overwritten by the contents of `bds_set` at run-time. > >This bug was introduced in 2017, when `bds_set` was converted to a >one-element array, and started to be used as a Variable Length Object >(VLO) at run-time. > >Fixes: f5823fe6897c ("qed: Add ll2 option to limit the number of bds per packet") >Cc: stable@vger.kernel.org >Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Kees Cook <keescook@chromium.org>
On Sat, Sep 23, 2023 at 07:15:59PM -0600, Gustavo A. R. Silva wrote: > The flexible structure (a structure that contains a flexible-array member > at the end) `qed_ll2_tx_packet` is nested within the second layer of > `struct qed_ll2_info`: > > struct qed_ll2_tx_packet { > ... > /* Flexible Array of bds_set determined by max_bds_per_packet */ > struct { > struct core_tx_bd *txq_bd; > dma_addr_t tx_frag; > u16 frag_len; > } bds_set[]; > }; > > struct qed_ll2_tx_queue { > ... > struct qed_ll2_tx_packet cur_completing_packet; > }; > > struct qed_ll2_info { > ... > struct qed_ll2_tx_queue tx_queue; > struct qed_ll2_cbs cbs; > }; > > The problem is that member `cbs` in `struct qed_ll2_info` is placed just > after an object of type `struct qed_ll2_tx_queue`, which is in itself > an implicit flexible structure, which by definition ends in a flexible > array member, in this case `bds_set`. This causes an undefined behavior > bug at run-time when dynamic memory is allocated for `bds_set`, which > could lead to a serious issue if `cbs` in `struct qed_ll2_info` is > overwritten by the contents of `bds_set`. Notice that the type of `cbs` > is a structure full of function pointers (and a cookie :) ): > > include/linux/qed/qed_ll2_if.h: > 107 typedef > 108 void (*qed_ll2_complete_rx_packet_cb)(void *cxt, > 109 struct qed_ll2_comp_rx_data *data); > 110 > 111 typedef > 112 void (*qed_ll2_release_rx_packet_cb)(void *cxt, > 113 u8 connection_handle, > 114 void *cookie, > 115 dma_addr_t rx_buf_addr, > 116 bool b_last_packet); > 117 > 118 typedef > 119 void (*qed_ll2_complete_tx_packet_cb)(void *cxt, > 120 u8 connection_handle, > 121 void *cookie, > 122 dma_addr_t first_frag_addr, > 123 bool b_last_fragment, > 124 bool b_last_packet); > 125 > 126 typedef > 127 void (*qed_ll2_release_tx_packet_cb)(void *cxt, > 128 u8 connection_handle, > 129 void *cookie, > 130 dma_addr_t first_frag_addr, > 131 bool b_last_fragment, bool b_last_packet); > 132 > 133 typedef > 134 void (*qed_ll2_slowpath_cb)(void *cxt, u8 connection_handle, > 135 u32 opaque_data_0, u32 opaque_data_1); > 136 > 137 struct qed_ll2_cbs { > 138 qed_ll2_complete_rx_packet_cb rx_comp_cb; > 139 qed_ll2_release_rx_packet_cb rx_release_cb; > 140 qed_ll2_complete_tx_packet_cb tx_comp_cb; > 141 qed_ll2_release_tx_packet_cb tx_release_cb; > 142 qed_ll2_slowpath_cb slowpath_cb; > 143 void *cookie; > 144 }; > > Fix this by moving the declaration of `cbs` to the middle of its > containing structure `qed_ll2_info`, preventing it from being > overwritten by the contents of `bds_set` at run-time. > > This bug was introduced in 2017, when `bds_set` was converted to a > one-element array, and started to be used as a Variable Length Object > (VLO) at run-time. > > Fixes: f5823fe6897c ("qed: Add ll2 option to limit the number of bds per packet") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sat, 23 Sep 2023 19:15:59 -0600 you wrote: > The flexible structure (a structure that contains a flexible-array member > at the end) `qed_ll2_tx_packet` is nested within the second layer of > `struct qed_ll2_info`: > > struct qed_ll2_tx_packet { > ... > /* Flexible Array of bds_set determined by max_bds_per_packet */ > struct { > struct core_tx_bd *txq_bd; > dma_addr_t tx_frag; > u16 frag_len; > } bds_set[]; > }; > > [...] Here is the summary with links: - qed/red_ll2: Fix undefined behavior bug in struct qed_ll2_info https://git.kernel.org/netdev/net/c/eea03d18af9c You are awesome, thank you!
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.h b/drivers/net/ethernet/qlogic/qed/qed_ll2.h index 0bfc375161ed..a174c6fc626a 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.h +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.h @@ -110,9 +110,9 @@ struct qed_ll2_info { enum core_tx_dest tx_dest; u8 tx_stats_en; bool main_func_queue; + struct qed_ll2_cbs cbs; struct qed_ll2_rx_queue rx_queue; struct qed_ll2_tx_queue tx_queue; - struct qed_ll2_cbs cbs; }; extern const struct qed_ll2_ops qed_ll2_ops_pass;
The flexible structure (a structure that contains a flexible-array member at the end) `qed_ll2_tx_packet` is nested within the second layer of `struct qed_ll2_info`: struct qed_ll2_tx_packet { ... /* Flexible Array of bds_set determined by max_bds_per_packet */ struct { struct core_tx_bd *txq_bd; dma_addr_t tx_frag; u16 frag_len; } bds_set[]; }; struct qed_ll2_tx_queue { ... struct qed_ll2_tx_packet cur_completing_packet; }; struct qed_ll2_info { ... struct qed_ll2_tx_queue tx_queue; struct qed_ll2_cbs cbs; }; The problem is that member `cbs` in `struct qed_ll2_info` is placed just after an object of type `struct qed_ll2_tx_queue`, which is in itself an implicit flexible structure, which by definition ends in a flexible array member, in this case `bds_set`. This causes an undefined behavior bug at run-time when dynamic memory is allocated for `bds_set`, which could lead to a serious issue if `cbs` in `struct qed_ll2_info` is overwritten by the contents of `bds_set`. Notice that the type of `cbs` is a structure full of function pointers (and a cookie :) ): include/linux/qed/qed_ll2_if.h: 107 typedef 108 void (*qed_ll2_complete_rx_packet_cb)(void *cxt, 109 struct qed_ll2_comp_rx_data *data); 110 111 typedef 112 void (*qed_ll2_release_rx_packet_cb)(void *cxt, 113 u8 connection_handle, 114 void *cookie, 115 dma_addr_t rx_buf_addr, 116 bool b_last_packet); 117 118 typedef 119 void (*qed_ll2_complete_tx_packet_cb)(void *cxt, 120 u8 connection_handle, 121 void *cookie, 122 dma_addr_t first_frag_addr, 123 bool b_last_fragment, 124 bool b_last_packet); 125 126 typedef 127 void (*qed_ll2_release_tx_packet_cb)(void *cxt, 128 u8 connection_handle, 129 void *cookie, 130 dma_addr_t first_frag_addr, 131 bool b_last_fragment, bool b_last_packet); 132 133 typedef 134 void (*qed_ll2_slowpath_cb)(void *cxt, u8 connection_handle, 135 u32 opaque_data_0, u32 opaque_data_1); 136 137 struct qed_ll2_cbs { 138 qed_ll2_complete_rx_packet_cb rx_comp_cb; 139 qed_ll2_release_rx_packet_cb rx_release_cb; 140 qed_ll2_complete_tx_packet_cb tx_comp_cb; 141 qed_ll2_release_tx_packet_cb tx_release_cb; 142 qed_ll2_slowpath_cb slowpath_cb; 143 void *cookie; 144 }; Fix this by moving the declaration of `cbs` to the middle of its containing structure `qed_ll2_info`, preventing it from being overwritten by the contents of `bds_set` at run-time. This bug was introduced in 2017, when `bds_set` was converted to a one-element array, and started to be used as a Variable Length Object (VLO) at run-time. Fixes: f5823fe6897c ("qed: Add ll2 option to limit the number of bds per packet") Cc: stable@vger.kernel.org Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/net/ethernet/qlogic/qed/qed_ll2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)