| Message ID | 20210323113413.29179-1-atulgopinathan@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] staging: rtl8192e: Fix incorrect source in memcpy() | expand |
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
This is very clever detective work. How did you spot the bug?
regards,
dan carpenter
On Tue, Mar 23, 2021 at 04:58:11PM +0300, Dan Carpenter wrote: > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com> > > This is very clever detective work. How did you spot the bug? I remember trying to investigate a sparse warning couple of weeks ago in the same file and went through the code when my eyes caught a struct pointer being indexed, which looked wierd. On further reading of the code and trying to make sense of what was happening (especially after calculating the size of the pointer) I became sure that it wasn't correct. The first patch led me to the second patch as they were related, "CcxRmState" was used in the same memcpy() of the first patch. I caught the error in it's type while trying to fix the first. Thank you for your encouraging words :D Regards, Atul
diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 8415f26fd4c0..6e48b31a9afc 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1965,15 +1965,15 @@ static void rtllib_parse_mife_generic(struct rtllib_device *ieee, if (info_element->len > 4 && info_element->data[0] == 0x00 && info_element->data[1] == 0x40 && info_element->data[2] == 0x96 && info_element->data[3] == 0x01) { if (info_element->len == 6) { - memcpy(network->CcxRmState, &info_element[4], 2); + memcpy(network->CcxRmState, &info_element->data[4], 2); if (network->CcxRmState[0] != 0) network->bCcxRmEnable = true; else network->bCcxRmEnable = false; network->MBssidMask = network->CcxRmState[1] & 0x07; if (network->MBssidMask != 0) { network->bMBssidValid = true;
The variable "info_element" is of the following type: struct rtllib_info_element *info_element defined in drivers/staging/rtl8192e/rtllib.h: struct rtllib_info_element { u8 id; u8 len; u8 data[]; } __packed; The "len" field defines the size of the "data[]" array. The code is supposed to check if "info_element->len" is greater than 4 and later equal to 6. If this is satisfied then, the last two bytes (the 4th and 5th element of u8 "data[]" array) are copied into "network->CcxRmState". Right now the code uses "memcpy()" with the source as "&info_element[4]" which would copy in wrong and unintended information. The struct "rtllib_info_element" has a size of 2 bytes for "id" and "len", therefore indexing will be done in interval of 2 bytes. So, "info_element[4]" would point to data which is beyond the memory allocated for this pointer (that is, at x+8, while "info_element" has been allocated only from x to x+7 (2 + 6 => 8 bytes)). This patch rectifies this error by using "&info_element->data[4]" which correctly copies the last two bytes of "data[]". NOTE: The faulty line of code came from the following commit: commit ecdfa44610fa ("Staging: add Realtek 8192 PCI wireless driver") The above commit created the file `rtl8192e/ieee80211/ieee80211_rx.c` which had the faulty line of code. This file has been deleted (or possibly renamed) with the contents copied in to a new file `rtl8192e/rtllib_rx.c` along with additional code in the commit 94a799425eee (tagged in Fixes). Fixes: 94a799425eee ("[PATCH 1/8] rtl8192e: Import new version of driver from realtek") Cc: stable@vger.kernel.org Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/rtl8192e/rtllib_rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)