| Message ID | 587423b0737108effe82aefed4407daca39e9a51.1692829410.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: mwifiex: Fix tlv_buf_left calculation and replace one-element array | expand |
On Wed, Aug 23, 2023 at 3:33 PM Gustavo A. R. Silva <gustavoars@kernel.org> wrote: > > Add sanity checks for both `tlv_len` and `tlv_bitmap_len` before > decoding data from `event_buf`. > > This prevents any malicious or buggy firmware from overflowing > `event_buf` through large values for `tlv_len` or `tlv_bitmap_len`. > > Suggested-by: Dan Williams <dcbw@redhat.com> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Justin Stitt <justinstitt@google.com> > --- > .../net/wireless/marvell/mwifiex/11n_rxreorder.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c > index 735aac52bdc4..9ee3b9f1e9ce 100644 > --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c > +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c > @@ -921,6 +921,14 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv, > while (tlv_buf_left > sizeof(*tlv_rxba)) { > tlv_type = le16_to_cpu(tlv_rxba->header.type); > tlv_len = le16_to_cpu(tlv_rxba->header.len); > + if (size_add(sizeof(tlv_rxba->header), tlv_len) > tlv_buf_left) { > + mwifiex_dbg(priv->adapter, WARN, > + "TLV size (%ld) overflows event_buf (%d)\n", > + size_add(sizeof(tlv_rxba->header), tlv_len), > + tlv_buf_left); > + return; > + } > + > if (tlv_type != TLV_TYPE_RXBA_SYNC) { > mwifiex_dbg(priv->adapter, ERROR, > "Wrong TLV id=0x%x\n", tlv_type); > @@ -929,6 +937,14 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv, > > tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num); > tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len); > + if (size_add(sizeof(*tlv_rxba), tlv_bitmap_len) > tlv_buf_left) { > + mwifiex_dbg(priv->adapter, WARN, > + "TLV size (%ld) overflows event_buf (%d)\n", > + size_add(sizeof(*tlv_rxba), tlv_bitmap_len), > + tlv_buf_left); > + return; > + } > + > mwifiex_dbg(priv->adapter, INFO, > "%pM tid=%d seq_num=%d bitmap_len=%d\n", > tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num, > -- > 2.34.1 >
Hi Gustavo, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.5-rc7 next-20230823] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Gustavo-A-R-Silva/wifi-mwifiex-Fix-tlv_buf_left-calculation/20230824-063517 base: linus/master patch link: https://lore.kernel.org/r/587423b0737108effe82aefed4407daca39e9a51.1692829410.git.gustavoars%40kernel.org patch subject: [PATCH 3/3] wifi: mwifiex: Sanity check tlv_len and tlv_bitmap_len config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230824/202308240844.leyoOwdG-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230824/202308240844.leyoOwdG-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308240844.leyoOwdG-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:12: drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c: In function 'mwifiex_11n_rxba_sync_event': >> drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:926:37: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=] 926 | "TLV size (%ld) overflows event_buf (%d)\n", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 927 | size_add(sizeof(tlv_rxba->header), tlv_len), | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | size_t {aka unsigned int} drivers/net/wireless/marvell/mwifiex/main.h:199:51: note: in definition of macro 'mwifiex_dbg' 199 | _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__) | ^~~ drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:926:50: note: format string is defined here 926 | "TLV size (%ld) overflows event_buf (%d)\n", | ~~^ | | | long int | %d drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:942:37: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=] 942 | "TLV size (%ld) overflows event_buf (%d)\n", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 943 | size_add(sizeof(*tlv_rxba), tlv_bitmap_len), | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | size_t {aka unsigned int} drivers/net/wireless/marvell/mwifiex/main.h:199:51: note: in definition of macro 'mwifiex_dbg' 199 | _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__) | ^~~ drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:942:50: note: format string is defined here 942 | "TLV size (%ld) overflows event_buf (%d)\n", | ~~^ | | | long int | %d vim +926 drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c 904 905 /* This function handles rxba_sync event 906 */ 907 void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv, 908 u8 *event_buf, u16 len) 909 { 910 struct mwifiex_ie_types_rxba_sync *tlv_rxba = (void *)event_buf; 911 u16 tlv_type, tlv_len; 912 struct mwifiex_rx_reorder_tbl *rx_reor_tbl_ptr; 913 u8 i, j; 914 u16 seq_num, tlv_seq_num, tlv_bitmap_len; 915 int tlv_buf_left = len; 916 int ret; 917 u8 *tmp; 918 919 mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:", 920 event_buf, len); 921 while (tlv_buf_left > sizeof(*tlv_rxba)) { 922 tlv_type = le16_to_cpu(tlv_rxba->header.type); 923 tlv_len = le16_to_cpu(tlv_rxba->header.len); 924 if (size_add(sizeof(tlv_rxba->header), tlv_len) > tlv_buf_left) { 925 mwifiex_dbg(priv->adapter, WARN, > 926 "TLV size (%ld) overflows event_buf (%d)\n",
diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c index 735aac52bdc4..9ee3b9f1e9ce 100644 --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c @@ -921,6 +921,14 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv, while (tlv_buf_left > sizeof(*tlv_rxba)) { tlv_type = le16_to_cpu(tlv_rxba->header.type); tlv_len = le16_to_cpu(tlv_rxba->header.len); + if (size_add(sizeof(tlv_rxba->header), tlv_len) > tlv_buf_left) { + mwifiex_dbg(priv->adapter, WARN, + "TLV size (%ld) overflows event_buf (%d)\n", + size_add(sizeof(tlv_rxba->header), tlv_len), + tlv_buf_left); + return; + } + if (tlv_type != TLV_TYPE_RXBA_SYNC) { mwifiex_dbg(priv->adapter, ERROR, "Wrong TLV id=0x%x\n", tlv_type); @@ -929,6 +937,14 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv, tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num); tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len); + if (size_add(sizeof(*tlv_rxba), tlv_bitmap_len) > tlv_buf_left) { + mwifiex_dbg(priv->adapter, WARN, + "TLV size (%ld) overflows event_buf (%d)\n", + size_add(sizeof(*tlv_rxba), tlv_bitmap_len), + tlv_buf_left); + return; + } + mwifiex_dbg(priv->adapter, INFO, "%pM tid=%d seq_num=%d bitmap_len=%d\n", tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
Add sanity checks for both `tlv_len` and `tlv_bitmap_len` before decoding data from `event_buf`. This prevents any malicious or buggy firmware from overflowing `event_buf` through large values for `tlv_len` or `tlv_bitmap_len`. Suggested-by: Dan Williams <dcbw@redhat.com> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- .../net/wireless/marvell/mwifiex/11n_rxreorder.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)