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.
Cc: stable@vger.kernel.org Addresses-Coverity-ID: 1461515 ("Out-of-bounds write") Fixes: e3037485c68ec ("rtw88: new Realtek 802.11ac driver") Signed-off-by: Len Baker len.baker@gmx.com --- Changelog v1 -> v2 - Remove the macro ARRAY_SIZE from the for loop (Pkshih, Brian Norris). - Add a new check for the len variable (Pkshih, Brian Norris).
drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index e7d17ab8f113..53dc90276693 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -273,6 +273,11 @@ static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev, return -EINVAL; }
+ if (len > ARRAY_SIZE(rx_ring->buf)) { + rtw_err(rtwdev, "len %d exceeds maximum RX ring buffer\n", len); + return -EINVAL; + } + head = dma_alloc_coherent(&pdev->dev, ring_sz, &dma, GFP_KERNEL); if (!head) { rtw_err(rtwdev, "failed to allocate rx ring\n"); -- 2.25.1
On Fri, Jul 16, 2021 at 8:54 AM Len Baker len.baker@gmx.com 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.
Cc: stable@vger.kernel.org
This kinda seems excessive, considering we absolutely know this is not currently a bug. But then, LWN nicely highlighted this thread, which reminds me that even without the Cc stable, this is likely to unnecessarily get picked up:
https://lwn.net/ml/linux-kernel/YO0zXVX9Bx9QZCTs@kroah.com/
And I guess silencing Coverity is a desirable goal in many cases, even if Coverity is being a bit trigger-happy.
So, *shrug*.
Addresses-Coverity-ID: 1461515 ("Out-of-bounds write") Fixes: e3037485c68ec ("rtw88: new Realtek 802.11ac driver") Signed-off-by: Len Baker len.baker@gmx.com
Changelog v1 -> v2
- Remove the macro ARRAY_SIZE from the for loop (Pkshih, Brian Norris).
- Add a new check for the len variable (Pkshih, Brian Norris).
Reviewed-by: Brian Norris briannorris@chromium.org
Thanks.
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. Why would you check twice for the same thing?
thanks,
greg k-h
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?
Thanks, Len
thanks,
greg k-h
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
On Sat, Jul 17, 2021 at 07:33:49PM +0200, Greg KH wrote:
On Sat, Jul 17, 2021 at 03:33:43PM +0200, Len Baker wrote:
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?
Ok, then I will remove the check and I will send a patch for review.
thanks,
greg k-h
Regards, Len
linux-stable-mirror@lists.linaro.org
-
Brian Norris -
Greg KH -
Len Baker