Re: [PATCH] qed/red_ll2: Fix undefined behavior bug in struct qed_ll2_info

[Simon Horman]

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@xxxxxxxxxxxxxxx
> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>