On Sat, Jul 17, 2021 at 03:33:43PM +0200, Len Baker wrote:
On Fri, Jul 16, 2021 at 07:20:48PM +0200, Greg KH wrote:
On Fri, Jul 16, 2021 at 05:53:11PM +0200, Len Baker wrote:
In the rtw_pci_init_rx_ring function the "if (len > TRX_BD_IDX_MASK)" statement guarantees that len is less than or equal to GENMASK(11, 0) or in other words that len is less than or equal to 4095. However the rx_ring->buf has a size of RTK_MAX_RX_DESC_NUM (defined as 512). This way it is possible an out-of-bounds write in the for statement due to the i variable can exceed the rx_ring->buff size.
However, this overflow never happens due to the rtw_pci_init_rx_ring is only ever called with a fixed constant of RTK_MAX_RX_DESC_NUM. But it is better to be defensive in this case and add a new check to avoid overflows if this function is called in a future with a value greater than 512.
If this can never happen, then no, this is not needed.
Then, if this can never happen, the current check would not be necessary either.
Why would you check twice for the same thing?
Ok, it makes no sense to double check the "len" variable twice. So, I propose to modify the current check as follows:
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index e7d17ab8f113..0fd140523868 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -268,8 +268,8 @@ static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev, int i, allocated; int ret = 0;
if (len > TRX_BD_IDX_MASK) {rtw_err(rtwdev, "len %d exceeds maximum RX entries\n", len);
if (len > ARRAY_SIZE(rx_ring->buf)) {rtw_err(rtwdev, "len %d exceeds maximum RX ring buffer\n", len); return -EINVAL; }This way the overflow can never happen with the current call to rtw_pci_init_rx_ring function or with a future call with a "len" parameter greater than 512. What do you think?
If there are no objections I will send a v3 for review.
Another question: If this can never happen should I include the "Fixes" tag, "Addresses-Coverity-ID" tag and Cc to stable?
If it can never happen, why have this check at all?
Looks like a Coverity false positive?
thanks,
greg k-h