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: [email protected]
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
drivers/net/ethernet/qlogic/qed/qed_ll2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
--
2.34.1
On September 23, 2023 6:15:59 PM PDT, "Gustavo A. R. Silva" <[email protected]> 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: [email protected]
>Signed-off-by: Gustavo A. R. Silva <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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: [email protected]
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
Reviewed-by: Simon Horman <[email protected]>