* [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer @ 2021-02-20 18:21 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-20 18:21 UTC (permalink / raw) To: gregkh; +Cc: gustavo, tiwai, devel, linux-kernel, Atul Gopinathan The variable "info_element" is of the following type: struct rtllib_info_element *info_element rtllib_info_element is a struct containing the following fields as defined in drivers/staging/rtl8192e/rtllib.h: struct rtllib_info_element { u8 id; u8 len; u8 data[]; } __packed; The following code of interest (to which this patch applies) is supposed to check if the "info_element->len" is greater than 4 and equal to 6, if this is satisfied then, the last two bytes (the 4th and 5th index of u8 "data" array) are copied into "network->CcxRmState". Currently the code uses "memcpy()" with the source as "&info_element[4]" which would copy in wrong and unintended information. This patch rectifies this error by using "&info_element->data[4]" which rightly copies the last two bytes as the required state information. Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/rtl8192e/rtllib_rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 66c135321da4..15bbb63ca130 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1963,15 +1963,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; -- 2.27.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer @ 2021-02-20 18:21 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-20 18:21 UTC (permalink / raw) To: gregkh; +Cc: tiwai, devel, Atul Gopinathan, linux-kernel, gustavo The variable "info_element" is of the following type: struct rtllib_info_element *info_element rtllib_info_element is a struct containing the following fields as defined in drivers/staging/rtl8192e/rtllib.h: struct rtllib_info_element { u8 id; u8 len; u8 data[]; } __packed; The following code of interest (to which this patch applies) is supposed to check if the "info_element->len" is greater than 4 and equal to 6, if this is satisfied then, the last two bytes (the 4th and 5th index of u8 "data" array) are copied into "network->CcxRmState". Currently the code uses "memcpy()" with the source as "&info_element[4]" which would copy in wrong and unintended information. This patch rectifies this error by using "&info_element->data[4]" which rightly copies the last two bytes as the required state information. Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/rtl8192e/rtllib_rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 66c135321da4..15bbb63ca130 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1963,15 +1963,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; -- 2.27.0 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-20 18:21 ` Atul Gopinathan @ 2021-02-20 18:21 ` Atul Gopinathan -1 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-20 18:21 UTC (permalink / raw) To: gregkh; +Cc: gustavo, tiwai, devel, linux-kernel, Atul Gopinathan The "CcxRmState" field in struct "rtllib_network" is defined as a u16 array of size 2 (so, 4 bytes in total). But the operations performed on this array throughout the code base (in rtl8192e/) are all in byte size 2 indicating that this array's type was defined wrongly. There are two situation were u16 type of this field could yield incorrect behaviour: 1. In rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); Here last 2 bytes (index 4 and 5) from the info_element->data[] array are meant to be copied into CcxRmState[]. Note that "data" array here is an array of type u8. 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: memcpy(dst->CcxRmState, src->CcxRmState, 2); Here again, only 2 bytes are copied from the source state to destination state. There are no instances of "CcxRmState" requiring u16 data type. Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ directory for reviewing: rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); rtllib.h:1108: u8 CcxRmState[2]; //Note: The last line of output is my commit to this patch (u8 instead of u16). Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/rtl8192e/rtllib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h index b84f00b8d18b..4cabaf21c1ca 100644 --- a/drivers/staging/rtl8192e/rtllib.h +++ b/drivers/staging/rtl8192e/rtllib.h @@ -1101,15 +1101,15 @@ struct rtllib_network { u8 hidden_ssid[IW_ESSID_MAX_SIZE + 1]; u8 hidden_ssid_len; struct rtllib_qos_data qos_data; bool bWithAironetIE; bool bCkipSupported; bool bCcxRmEnable; - u16 CcxRmState[2]; + u8 CcxRmState[2]; bool bMBssidValid; u8 MBssidMask; u8 MBssid[ETH_ALEN]; bool bWithCcxVerNum; u8 BssCcxVerNumber; /* These are network statistics */ struct rtllib_rx_stats stats; -- 2.27.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-02-20 18:21 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-20 18:21 UTC (permalink / raw) To: gregkh; +Cc: tiwai, devel, Atul Gopinathan, linux-kernel, gustavo The "CcxRmState" field in struct "rtllib_network" is defined as a u16 array of size 2 (so, 4 bytes in total). But the operations performed on this array throughout the code base (in rtl8192e/) are all in byte size 2 indicating that this array's type was defined wrongly. There are two situation were u16 type of this field could yield incorrect behaviour: 1. In rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); Here last 2 bytes (index 4 and 5) from the info_element->data[] array are meant to be copied into CcxRmState[]. Note that "data" array here is an array of type u8. 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: memcpy(dst->CcxRmState, src->CcxRmState, 2); Here again, only 2 bytes are copied from the source state to destination state. There are no instances of "CcxRmState" requiring u16 data type. Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ directory for reviewing: rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); rtllib.h:1108: u8 CcxRmState[2]; //Note: The last line of output is my commit to this patch (u8 instead of u16). Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/rtl8192e/rtllib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h index b84f00b8d18b..4cabaf21c1ca 100644 --- a/drivers/staging/rtl8192e/rtllib.h +++ b/drivers/staging/rtl8192e/rtllib.h @@ -1101,15 +1101,15 @@ struct rtllib_network { u8 hidden_ssid[IW_ESSID_MAX_SIZE + 1]; u8 hidden_ssid_len; struct rtllib_qos_data qos_data; bool bWithAironetIE; bool bCkipSupported; bool bCcxRmEnable; - u16 CcxRmState[2]; + u8 CcxRmState[2]; bool bMBssidValid; u8 MBssidMask; u8 MBssid[ETH_ALEN]; bool bWithCcxVerNum; u8 BssCcxVerNumber; /* These are network statistics */ struct rtllib_rx_stats stats; -- 2.27.0 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-20 18:21 ` Atul Gopinathan @ 2021-02-21 13:08 ` Greg KH -1 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-02-21 13:08 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > The "CcxRmState" field in struct "rtllib_network" is defined > as a u16 array of size 2 (so, 4 bytes in total). > > But the operations performed on this array throughout the code > base (in rtl8192e/) are all in byte size 2 indicating that this > array's type was defined wrongly. > > There are two situation were u16 type of this field could yield > incorrect behaviour: > > 1. In rtllib_rx.c:1970: > memcpy(network->CcxRmState, &info_element->data[4], 2); > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > array are meant to be copied into CcxRmState[]. > Note that "data" array here is an array of type u8. > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > Here again, only 2 bytes are copied from the source state to > destination state. > > There are no instances of "CcxRmState" requiring u16 data type. > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > directory for reviewing: > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > rtllib.h:1108: u8 CcxRmState[2]; You just changed the logic in line 1975 in that file, right? Are you _SURE_ that is ok? Do you have a device to test this on? thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-02-21 13:08 ` Greg KH 0 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-02-21 13:08 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > The "CcxRmState" field in struct "rtllib_network" is defined > as a u16 array of size 2 (so, 4 bytes in total). > > But the operations performed on this array throughout the code > base (in rtl8192e/) are all in byte size 2 indicating that this > array's type was defined wrongly. > > There are two situation were u16 type of this field could yield > incorrect behaviour: > > 1. In rtllib_rx.c:1970: > memcpy(network->CcxRmState, &info_element->data[4], 2); > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > array are meant to be copied into CcxRmState[]. > Note that "data" array here is an array of type u8. > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > Here again, only 2 bytes are copied from the source state to > destination state. > > There are no instances of "CcxRmState" requiring u16 data type. > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > directory for reviewing: > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > rtllib.h:1108: u8 CcxRmState[2]; You just changed the logic in line 1975 in that file, right? Are you _SURE_ that is ok? Do you have a device to test this on? thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-21 13:08 ` Greg KH @ 2021-02-21 16:57 ` Atul Gopinathan -1 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-21 16:57 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > The "CcxRmState" field in struct "rtllib_network" is defined > > as a u16 array of size 2 (so, 4 bytes in total). > > > > But the operations performed on this array throughout the code > > base (in rtl8192e/) are all in byte size 2 indicating that this > > array's type was defined wrongly. > > > > There are two situation were u16 type of this field could yield > > incorrect behaviour: > > > > 1. In rtllib_rx.c:1970: > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > array are meant to be copied into CcxRmState[]. > > Note that "data" array here is an array of type u8. > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > Here again, only 2 bytes are copied from the source state to > > destination state. > > > > There are no instances of "CcxRmState" requiring u16 data type. > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > directory for reviewing: > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > rtllib.h:1108: u8 CcxRmState[2]; > > You just changed the logic in line 1975 in that file, right? Are you > _SURE_ that is ok? Do you have a device to test this on? I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean the following line?: network->MBssidMask = network->CcxRmState[1] & 0x07; network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as seen above). I believe my patch doesn't change the logic of an "&" operation being performed on it with 0x07, right? (I'm sorry if I'm missing something quite obvious, could kindly elaborate the change in logic that you are referring to?) Many thanks for the reviewing it during this time! Atul ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-02-21 16:57 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-21 16:57 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > The "CcxRmState" field in struct "rtllib_network" is defined > > as a u16 array of size 2 (so, 4 bytes in total). > > > > But the operations performed on this array throughout the code > > base (in rtl8192e/) are all in byte size 2 indicating that this > > array's type was defined wrongly. > > > > There are two situation were u16 type of this field could yield > > incorrect behaviour: > > > > 1. In rtllib_rx.c:1970: > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > array are meant to be copied into CcxRmState[]. > > Note that "data" array here is an array of type u8. > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > Here again, only 2 bytes are copied from the source state to > > destination state. > > > > There are no instances of "CcxRmState" requiring u16 data type. > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > directory for reviewing: > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > rtllib.h:1108: u8 CcxRmState[2]; > > You just changed the logic in line 1975 in that file, right? Are you > _SURE_ that is ok? Do you have a device to test this on? I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean the following line?: network->MBssidMask = network->CcxRmState[1] & 0x07; network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as seen above). I believe my patch doesn't change the logic of an "&" operation being performed on it with 0x07, right? (I'm sorry if I'm missing something quite obvious, could kindly elaborate the change in logic that you are referring to?) Many thanks for the reviewing it during this time! Atul _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-21 16:57 ` Atul Gopinathan @ 2021-02-22 15:26 ` Greg KH -1 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-02-22 15:26 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > But the operations performed on this array throughout the code > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > array's type was defined wrongly. > > > > > > There are two situation were u16 type of this field could yield > > > incorrect behaviour: > > > > > > 1. In rtllib_rx.c:1970: > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > array are meant to be copied into CcxRmState[]. > > > Note that "data" array here is an array of type u8. > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > Here again, only 2 bytes are copied from the source state to > > > destination state. > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > directory for reviewing: > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > You just changed the logic in line 1975 in that file, right? Are you > > _SURE_ that is ok? Do you have a device to test this on? > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > the following line?: > > network->MBssidMask = network->CcxRmState[1] & 0x07; Yes. > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > seen above). I believe my patch doesn't change the logic of an "&" operation > being performed on it with 0x07, right? It changes the location of the [1] operation to point to a different place in memory from what I can tell, as you changed the type of that array. thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-02-22 15:26 ` Greg KH 0 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-02-22 15:26 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > But the operations performed on this array throughout the code > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > array's type was defined wrongly. > > > > > > There are two situation were u16 type of this field could yield > > > incorrect behaviour: > > > > > > 1. In rtllib_rx.c:1970: > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > array are meant to be copied into CcxRmState[]. > > > Note that "data" array here is an array of type u8. > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > Here again, only 2 bytes are copied from the source state to > > > destination state. > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > directory for reviewing: > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > You just changed the logic in line 1975 in that file, right? Are you > > _SURE_ that is ok? Do you have a device to test this on? > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > the following line?: > > network->MBssidMask = network->CcxRmState[1] & 0x07; Yes. > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > seen above). I believe my patch doesn't change the logic of an "&" operation > being performed on it with 0x07, right? It changes the location of the [1] operation to point to a different place in memory from what I can tell, as you changed the type of that array. thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-22 15:26 ` Greg KH @ 2021-02-22 17:23 ` Atul Gopinathan -1 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-22 17:23 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > But the operations performed on this array throughout the code > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > array's type was defined wrongly. > > > > > > > > There are two situation were u16 type of this field could yield > > > > incorrect behaviour: > > > > > > > > 1. In rtllib_rx.c:1970: > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > array are meant to be copied into CcxRmState[]. > > > > Note that "data" array here is an array of type u8. > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > destination state. > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > directory for reviewing: > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > _SURE_ that is ok? Do you have a device to test this on? > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > the following line?: > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > Yes. > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > seen above). I believe my patch doesn't change the logic of an "&" operation > > being performed on it with 0x07, right? > > It changes the location of the [1] operation to point to a different > place in memory from what I can tell, as you changed the type of that > array. Oh yes, earlier, the network->CcxRmState[] array had memory locations as: [x, x+16]. With this patch, it's locations are [x, x+8]. And I strongly believe this is how it should be based on how the original author is using the CcxRmState[] array throughout the codebase: Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that I sent previously): 1. At line 1970: memcpy(network->CcxRmState, &info_element->data[4], 2); this is where the array CcxRmState[] is being fed with data. And one can see the source is an array named "data" which itself has type u8. The third argument is "2", meaning 2 bytes of data should be written from "data" array to "CcxRmState". Also note that, the array CcxRmState has a size 2, as defined in rtllib.h, in struct "rtllib_network": u16 CcxRmState[2]; Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements of the source "data" array will only be written into the first element of "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will never be fed with any data. The resultant CcxRmState[] array would look something like this: [(u8-data and u8-data squashed), 0]. The 2 u8-data here refers to info_element->data[4] and info_element->data[5]. Instead, if "CcxRmState" was of type u8, then both elements of source "data" array will be written into the 2 elements of "CcxRmState" respectively: [u8 data, u8 data] This makes a lot more sense. 2. Line 1975: network->MBssidMask = network->CcxRmState[1] & 0x07; With point 1 clear, it should now be easy to understand that the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" is u16, simply because CcxRmState[1] is never fed with any data at all. Oh and "network->MBssidMask" is also of type u8. 3. Line 2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); 2 bytes, and not 4, again.:D The above line belongs to the following function: static inline void update_network(struct rtllib_device *ieee, struct rtllib_network *dst, struct rtllib_network *src) As you can see, there is "dst" destination and a "src" source. The author is essentially copying all the data from "src" to "dst" in this function. Throughout the function, "memcpy()" is being used several times to copy the data of all arrays/structs existing in "src" into "dst". In each of those instances, the author is making sure to copy the entirety of the respective struct/array by passing all used up size of the struct/ array in the third, size, argument. Here are a few lines from that function (posting the entire function defintion would be inappropriate) instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); There are a LOT more instances, here is the elixir link to that function for a quick reference: https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 My point is, it's clear that the intent of this function is to duplicate the data of src into dst. If "CcxRmState" really is supposed to be u16, then why only write down the first 2 bytes into "dst->CcxRmState"? What about "dst->CcxRmState[1]"? It never gets any value, again. These are the only places where CcxRmState is being used in the entire rtl8192e driver directory. I skipped line 1971 as it just checks whether "CcxRmState[0]" is 0 or not, this should not require any explanation. Thank you for your patience! Atul ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-02-22 17:23 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-22 17:23 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > But the operations performed on this array throughout the code > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > array's type was defined wrongly. > > > > > > > > There are two situation were u16 type of this field could yield > > > > incorrect behaviour: > > > > > > > > 1. In rtllib_rx.c:1970: > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > array are meant to be copied into CcxRmState[]. > > > > Note that "data" array here is an array of type u8. > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > destination state. > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > directory for reviewing: > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > _SURE_ that is ok? Do you have a device to test this on? > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > the following line?: > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > Yes. > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > seen above). I believe my patch doesn't change the logic of an "&" operation > > being performed on it with 0x07, right? > > It changes the location of the [1] operation to point to a different > place in memory from what I can tell, as you changed the type of that > array. Oh yes, earlier, the network->CcxRmState[] array had memory locations as: [x, x+16]. With this patch, it's locations are [x, x+8]. And I strongly believe this is how it should be based on how the original author is using the CcxRmState[] array throughout the codebase: Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that I sent previously): 1. At line 1970: memcpy(network->CcxRmState, &info_element->data[4], 2); this is where the array CcxRmState[] is being fed with data. And one can see the source is an array named "data" which itself has type u8. The third argument is "2", meaning 2 bytes of data should be written from "data" array to "CcxRmState". Also note that, the array CcxRmState has a size 2, as defined in rtllib.h, in struct "rtllib_network": u16 CcxRmState[2]; Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements of the source "data" array will only be written into the first element of "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will never be fed with any data. The resultant CcxRmState[] array would look something like this: [(u8-data and u8-data squashed), 0]. The 2 u8-data here refers to info_element->data[4] and info_element->data[5]. Instead, if "CcxRmState" was of type u8, then both elements of source "data" array will be written into the 2 elements of "CcxRmState" respectively: [u8 data, u8 data] This makes a lot more sense. 2. Line 1975: network->MBssidMask = network->CcxRmState[1] & 0x07; With point 1 clear, it should now be easy to understand that the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" is u16, simply because CcxRmState[1] is never fed with any data at all. Oh and "network->MBssidMask" is also of type u8. 3. Line 2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); 2 bytes, and not 4, again.:D The above line belongs to the following function: static inline void update_network(struct rtllib_device *ieee, struct rtllib_network *dst, struct rtllib_network *src) As you can see, there is "dst" destination and a "src" source. The author is essentially copying all the data from "src" to "dst" in this function. Throughout the function, "memcpy()" is being used several times to copy the data of all arrays/structs existing in "src" into "dst". In each of those instances, the author is making sure to copy the entirety of the respective struct/array by passing all used up size of the struct/ array in the third, size, argument. Here are a few lines from that function (posting the entire function defintion would be inappropriate) instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); There are a LOT more instances, here is the elixir link to that function for a quick reference: https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 My point is, it's clear that the intent of this function is to duplicate the data of src into dst. If "CcxRmState" really is supposed to be u16, then why only write down the first 2 bytes into "dst->CcxRmState"? What about "dst->CcxRmState[1]"? It never gets any value, again. These are the only places where CcxRmState is being used in the entire rtl8192e driver directory. I skipped line 1971 as it just checks whether "CcxRmState[0]" is 0 or not, this should not require any explanation. Thank you for your patience! Atul _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-02-22 17:23 ` Atul Gopinathan @ 2021-03-02 14:38 ` Greg KH -1 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-03-02 14:38 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Mon, Feb 22, 2021 at 10:53:30PM +0530, Atul Gopinathan wrote: > On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > > > But the operations performed on this array throughout the code > > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > > array's type was defined wrongly. > > > > > > > > > > There are two situation were u16 type of this field could yield > > > > > incorrect behaviour: > > > > > > > > > > 1. In rtllib_rx.c:1970: > > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > > array are meant to be copied into CcxRmState[]. > > > > > Note that "data" array here is an array of type u8. > > > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > > destination state. > > > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > > directory for reviewing: > > > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > > _SURE_ that is ok? Do you have a device to test this on? > > > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > > the following line?: > > > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > Yes. > > > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > > seen above). I believe my patch doesn't change the logic of an "&" operation > > > being performed on it with 0x07, right? > > > > It changes the location of the [1] operation to point to a different > > place in memory from what I can tell, as you changed the type of that > > array. > > Oh yes, earlier, the network->CcxRmState[] array had memory locations as: > [x, x+16]. With this patch, it's locations are [x, x+8]. > > And I strongly believe this is how it should be based on how the original > author is using the CcxRmState[] array throughout the codebase: Ok, but this has changed the way memory is addressed, which is what I was trying to point out when you said that nothing had changed. > Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that > I sent previously): > 1. At line 1970: > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > this is where the array CcxRmState[] is being fed with > data. And one can see the source is an array named "data" which itself > has type u8. The third argument is "2", meaning 2 bytes of data should > be written from "data" array to "CcxRmState". > > Also note that, the array CcxRmState has a size 2, as defined in > rtllib.h, in struct "rtllib_network": > > u16 CcxRmState[2]; > > Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements > of the source "data" array will only be written into the first element of > "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will > never be fed with any data. The resultant CcxRmState[] array would look > something like this: > > [(u8-data and u8-data squashed), 0]. > > The 2 u8-data here refers to info_element->data[4] and > info_element->data[5]. > > Instead, if "CcxRmState" was of type u8, then both elements of source > "data" array will be written into the 2 elements of "CcxRmState" > respectively: > > [u8 data, u8 data] > > This makes a lot more sense. > > 2. Line 1975: > network->MBssidMask = network->CcxRmState[1] & 0x07; > > With point 1 clear, it should now be easy to understand that > the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" > is u16, simply because CcxRmState[1] is never fed with any data at > all. > > Oh and "network->MBssidMask" is also of type u8. > > 3. Line 2520: > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > 2 bytes, and not 4, again.:D > The above line belongs to the following function: > > static inline void update_network(struct rtllib_device *ieee, > struct rtllib_network *dst, > struct rtllib_network *src) > > As you can see, there is "dst" destination and a "src" source. The author > is essentially copying all the data from "src" to "dst" in this function. > Throughout the function, "memcpy()" is being used several times to copy > the data of all arrays/structs existing in "src" into "dst". In each > of those instances, the author is making sure to copy the entirety of > the respective struct/array by passing all used up size of the struct/ > array in the third, size, argument. Here are a few lines from that > function (posting the entire function defintion would be inappropriate) > > instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); > instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); > instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); > instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); > > There are a LOT more instances, here is the elixir link to that > function for a quick reference: > https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 > > My point is, it's clear that the intent of this function is to duplicate > the data of src into dst. If "CcxRmState" really is supposed to be u16, > then why only write down the first 2 bytes into "dst->CcxRmState"? > > What about "dst->CcxRmState[1]"? It never gets any value, again. > > These are the only places where CcxRmState is being used in the entire > rtl8192e driver directory. I skipped line 1971 as it just checks whether > "CcxRmState[0]" is 0 or not, this should not require any explanation. Ok, can you provide more information in the changelog text and resend the commit so that it is easier to understand why you feel this change is ok? thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-03-02 14:38 ` Greg KH 0 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2021-03-02 14:38 UTC (permalink / raw) To: Atul Gopinathan; +Cc: tiwai, devel, linux-kernel, gustavo On Mon, Feb 22, 2021 at 10:53:30PM +0530, Atul Gopinathan wrote: > On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > > > But the operations performed on this array throughout the code > > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > > array's type was defined wrongly. > > > > > > > > > > There are two situation were u16 type of this field could yield > > > > > incorrect behaviour: > > > > > > > > > > 1. In rtllib_rx.c:1970: > > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > > array are meant to be copied into CcxRmState[]. > > > > > Note that "data" array here is an array of type u8. > > > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > > destination state. > > > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > > directory for reviewing: > > > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > > _SURE_ that is ok? Do you have a device to test this on? > > > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > > the following line?: > > > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > Yes. > > > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > > seen above). I believe my patch doesn't change the logic of an "&" operation > > > being performed on it with 0x07, right? > > > > It changes the location of the [1] operation to point to a different > > place in memory from what I can tell, as you changed the type of that > > array. > > Oh yes, earlier, the network->CcxRmState[] array had memory locations as: > [x, x+16]. With this patch, it's locations are [x, x+8]. > > And I strongly believe this is how it should be based on how the original > author is using the CcxRmState[] array throughout the codebase: Ok, but this has changed the way memory is addressed, which is what I was trying to point out when you said that nothing had changed. > Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that > I sent previously): > 1. At line 1970: > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > this is where the array CcxRmState[] is being fed with > data. And one can see the source is an array named "data" which itself > has type u8. The third argument is "2", meaning 2 bytes of data should > be written from "data" array to "CcxRmState". > > Also note that, the array CcxRmState has a size 2, as defined in > rtllib.h, in struct "rtllib_network": > > u16 CcxRmState[2]; > > Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements > of the source "data" array will only be written into the first element of > "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will > never be fed with any data. The resultant CcxRmState[] array would look > something like this: > > [(u8-data and u8-data squashed), 0]. > > The 2 u8-data here refers to info_element->data[4] and > info_element->data[5]. > > Instead, if "CcxRmState" was of type u8, then both elements of source > "data" array will be written into the 2 elements of "CcxRmState" > respectively: > > [u8 data, u8 data] > > This makes a lot more sense. > > 2. Line 1975: > network->MBssidMask = network->CcxRmState[1] & 0x07; > > With point 1 clear, it should now be easy to understand that > the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" > is u16, simply because CcxRmState[1] is never fed with any data at > all. > > Oh and "network->MBssidMask" is also of type u8. > > 3. Line 2520: > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > 2 bytes, and not 4, again.:D > The above line belongs to the following function: > > static inline void update_network(struct rtllib_device *ieee, > struct rtllib_network *dst, > struct rtllib_network *src) > > As you can see, there is "dst" destination and a "src" source. The author > is essentially copying all the data from "src" to "dst" in this function. > Throughout the function, "memcpy()" is being used several times to copy > the data of all arrays/structs existing in "src" into "dst". In each > of those instances, the author is making sure to copy the entirety of > the respective struct/array by passing all used up size of the struct/ > array in the third, size, argument. Here are a few lines from that > function (posting the entire function defintion would be inappropriate) > > instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); > instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); > instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); > instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); > > There are a LOT more instances, here is the elixir link to that > function for a quick reference: > https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 > > My point is, it's clear that the intent of this function is to duplicate > the data of src into dst. If "CcxRmState" really is supposed to be u16, > then why only write down the first 2 bytes into "dst->CcxRmState"? > > What about "dst->CcxRmState[1]"? It never gets any value, again. > > These are the only places where CcxRmState is being used in the entire > rtl8192e driver directory. I skipped line 1971 as it just checks whether > "CcxRmState[0]" is 0 or not, this should not require any explanation. Ok, can you provide more information in the changelog text and resend the commit so that it is easier to understand why you feel this change is ok? thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 2021-03-02 14:38 ` Greg KH @ 2021-03-02 16:53 ` Atul Gopinathan -1 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-03-02 16:53 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Tue, Mar 02, 2021 at 03:38:52PM +0100, Greg KH wrote: > On Mon, Feb 22, 2021 at 10:53:30PM +0530, Atul Gopinathan wrote: > > On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > > > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > > > > > But the operations performed on this array throughout the code > > > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > > > array's type was defined wrongly. > > > > > > > > > > > > There are two situation were u16 type of this field could yield > > > > > > incorrect behaviour: > > > > > > > > > > > > 1. In rtllib_rx.c:1970: > > > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > > > array are meant to be copied into CcxRmState[]. > > > > > > Note that "data" array here is an array of type u8. > > > > > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > > > destination state. > > > > > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > > > directory for reviewing: > > > > > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > > > _SURE_ that is ok? Do you have a device to test this on? > > > > > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > > > the following line?: > > > > > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > > Yes. > > > > > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > > > seen above). I believe my patch doesn't change the logic of an "&" operation > > > > being performed on it with 0x07, right? > > > > > > It changes the location of the [1] operation to point to a different > > > place in memory from what I can tell, as you changed the type of that > > > array. > > > > Oh yes, earlier, the network->CcxRmState[] array had memory locations as: > > [x, x+16]. With this patch, it's locations are [x, x+8]. > > > > And I strongly believe this is how it should be based on how the original > > author is using the CcxRmState[] array throughout the codebase: > > Ok, but this has changed the way memory is addressed, which is what I > was trying to point out when you said that nothing had changed. Ah sorry about that, It just wasn't clear to me what you meant and my mind was too fixated on the "&" operation. > > > Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that > > I sent previously): > > 1. At line 1970: > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > this is where the array CcxRmState[] is being fed with > > data. And one can see the source is an array named "data" which itself > > has type u8. The third argument is "2", meaning 2 bytes of data should > > be written from "data" array to "CcxRmState". > > > > Also note that, the array CcxRmState has a size 2, as defined in > > rtllib.h, in struct "rtllib_network": > > > > u16 CcxRmState[2]; > > > > Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements > > of the source "data" array will only be written into the first element of > > "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will > > never be fed with any data. The resultant CcxRmState[] array would look > > something like this: > > > > [(u8-data and u8-data squashed), 0]. > > > > The 2 u8-data here refers to info_element->data[4] and > > info_element->data[5]. > > > > Instead, if "CcxRmState" was of type u8, then both elements of source > > "data" array will be written into the 2 elements of "CcxRmState" > > respectively: > > > > [u8 data, u8 data] > > > > This makes a lot more sense. > > > > 2. Line 1975: > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > With point 1 clear, it should now be easy to understand that > > the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" > > is u16, simply because CcxRmState[1] is never fed with any data at > > all. > > > > Oh and "network->MBssidMask" is also of type u8. > > > > 3. Line 2520: > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > 2 bytes, and not 4, again.:D > > The above line belongs to the following function: > > > > static inline void update_network(struct rtllib_device *ieee, > > struct rtllib_network *dst, > > struct rtllib_network *src) > > > > As you can see, there is "dst" destination and a "src" source. The author > > is essentially copying all the data from "src" to "dst" in this function. > > Throughout the function, "memcpy()" is being used several times to copy > > the data of all arrays/structs existing in "src" into "dst". In each > > of those instances, the author is making sure to copy the entirety of > > the respective struct/array by passing all used up size of the struct/ > > array in the third, size, argument. Here are a few lines from that > > function (posting the entire function defintion would be inappropriate) > > > > instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); > > instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); > > instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); > > instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); > > > > There are a LOT more instances, here is the elixir link to that > > function for a quick reference: > > https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 > > > > My point is, it's clear that the intent of this function is to duplicate > > the data of src into dst. If "CcxRmState" really is supposed to be u16, > > then why only write down the first 2 bytes into "dst->CcxRmState"? > > > > What about "dst->CcxRmState[1]"? It never gets any value, again. > > > > These are the only places where CcxRmState is being used in the entire > > rtl8192e driver directory. I skipped line 1971 as it just checks whether > > "CcxRmState[0]" is 0 or not, this should not require any explanation. > > Ok, can you provide more information in the changelog text and resend > the commit so that it is easier to understand why you feel this change > is ok? Sure! Just a couple of doubts: 1. Do you want me to include all the information I wrote in my previous e-mail into the body of new e-mail? (Is there a limit to how long the body of the mail should be?) 2. I was asked to add the Fixes tag and also tag in -stable in my patch. I had a small confusion with the "Fixes" tag which I asked earlier in the thread but couldn't get it clarified with anyone, so I'll ask again: The previous commit pertaining to the lines I'm modifying is a checkpatch.pl fix (found using simple "git blame"). Should I write _that_ as the Fixes <Commit ID>? Or should I write in the commit id which created that file and hence, that specific line? (which makes more sense.) `git blame -L1960,1980 -- rtllib_rx.c` returns a single commit (80d2579d8608f) which was a checkpatch fix (1970, is the line my patch-1/1 modifies `git log -S'&info_element[4]' -- rtllib_rx.c` returned the commit (94a799425eee8) which created the file (the file which my patch-1/1 modifies) Which one should I write in the Fixes tag? Thanks for the review! Atul ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 @ 2021-03-02 16:53 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-03-02 16:53 UTC (permalink / raw) To: Greg KH; +Cc: tiwai, devel, linux-kernel, gustavo On Tue, Mar 02, 2021 at 03:38:52PM +0100, Greg KH wrote: > On Mon, Feb 22, 2021 at 10:53:30PM +0530, Atul Gopinathan wrote: > > On Mon, Feb 22, 2021 at 04:26:33PM +0100, Greg KH wrote: > > > On Sun, Feb 21, 2021 at 10:27:21PM +0530, Atul Gopinathan wrote: > > > > On Sun, Feb 21, 2021 at 02:08:26PM +0100, Greg KH wrote: > > > > > On Sat, Feb 20, 2021 at 11:51:55PM +0530, Atul Gopinathan wrote: > > > > > > The "CcxRmState" field in struct "rtllib_network" is defined > > > > > > as a u16 array of size 2 (so, 4 bytes in total). > > > > > > > > > > > > But the operations performed on this array throughout the code > > > > > > base (in rtl8192e/) are all in byte size 2 indicating that this > > > > > > array's type was defined wrongly. > > > > > > > > > > > > There are two situation were u16 type of this field could yield > > > > > > incorrect behaviour: > > > > > > > > > > > > 1. In rtllib_rx.c:1970: > > > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > > > > > > > Here last 2 bytes (index 4 and 5) from the info_element->data[] > > > > > > array are meant to be copied into CcxRmState[]. > > > > > > Note that "data" array here is an array of type u8. > > > > > > > > > > > > 2. In function "update_network()" in staging/rtl8192e/rtllib_rx.c: > > > > > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > > > > > > > Here again, only 2 bytes are copied from the source state to > > > > > > destination state. > > > > > > > > > > > > There are no instances of "CcxRmState" requiring u16 data type. > > > > > > Here is the output of "grep -IRn 'CcxRmState'" on the rtl8192e/ > > > > > > directory for reviewing: > > > > > > > > > > > > rtllib_rx.c:1970: memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > > > rtllib_rx.c:1971: if (network->CcxRmState[0] != 0) > > > > > > rtllib_rx.c:1975: network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > > rtllib_rx.c:2520: memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > > > rtllib.h:1108: u8 CcxRmState[2]; > > > > > > > > > > You just changed the logic in line 1975 in that file, right? Are you > > > > > _SURE_ that is ok? Do you have a device to test this on? > > > > > > > > I'm sorry, I didn't quite get you. By line 1975 in rtllib_rx.c, did you mean > > > > the following line?: > > > > > > > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > > > Yes. > > > > > > > network->CcxRmState is being fed with 2 bytes of u8 data, in line 1970 (as > > > > seen above). I believe my patch doesn't change the logic of an "&" operation > > > > being performed on it with 0x07, right? > > > > > > It changes the location of the [1] operation to point to a different > > > place in memory from what I can tell, as you changed the type of that > > > array. > > > > Oh yes, earlier, the network->CcxRmState[] array had memory locations as: > > [x, x+16]. With this patch, it's locations are [x, x+8]. > > > > And I strongly believe this is how it should be based on how the original > > author is using the CcxRmState[] array throughout the codebase: > > Ok, but this has changed the way memory is addressed, which is what I > was trying to point out when you said that nothing had changed. Ah sorry about that, It just wasn't clear to me what you meant and my mind was too fixated on the "&" operation. > > > Allow me to explain (Based on the output of "grep -IRn 'CcxRmState'" that > > I sent previously): > > 1. At line 1970: > > > > memcpy(network->CcxRmState, &info_element->data[4], 2); > > > > this is where the array CcxRmState[] is being fed with > > data. And one can see the source is an array named "data" which itself > > has type u8. The third argument is "2", meaning 2 bytes of data should > > be written from "data" array to "CcxRmState". > > > > Also note that, the array CcxRmState has a size 2, as defined in > > rtllib.h, in struct "rtllib_network": > > > > u16 CcxRmState[2]; > > > > Say if CcxRmState[] _was_ supposed to be u16 and not u8, then both elements > > of the source "data" array will only be written into the first element of > > "CcxRmState", i.e, "CcxRmState[0]". The 2nd element, "CcxRmState[1]" will > > never be fed with any data. The resultant CcxRmState[] array would look > > something like this: > > > > [(u8-data and u8-data squashed), 0]. > > > > The 2 u8-data here refers to info_element->data[4] and > > info_element->data[5]. > > > > Instead, if "CcxRmState" was of type u8, then both elements of source > > "data" array will be written into the 2 elements of "CcxRmState" > > respectively: > > > > [u8 data, u8 data] > > > > This makes a lot more sense. > > > > 2. Line 1975: > > network->MBssidMask = network->CcxRmState[1] & 0x07; > > > > With point 1 clear, it should now be easy to understand that > > the the "&" operation in line 1975, will _always_ yield 0 if "CcxRmState" > > is u16, simply because CcxRmState[1] is never fed with any data at > > all. > > > > Oh and "network->MBssidMask" is also of type u8. > > > > 3. Line 2520: > > memcpy(dst->CcxRmState, src->CcxRmState, 2); > > > > 2 bytes, and not 4, again.:D > > The above line belongs to the following function: > > > > static inline void update_network(struct rtllib_device *ieee, > > struct rtllib_network *dst, > > struct rtllib_network *src) > > > > As you can see, there is "dst" destination and a "src" source. The author > > is essentially copying all the data from "src" to "dst" in this function. > > Throughout the function, "memcpy()" is being used several times to copy > > the data of all arrays/structs existing in "src" into "dst". In each > > of those instances, the author is making sure to copy the entirety of > > the respective struct/array by passing all used up size of the struct/ > > array in the third, size, argument. Here are a few lines from that > > function (posting the entire function defintion would be inappropriate) > > > > instance 1: memcpy(dst->hidden_ssid, src->ssid, src->ssid_len); > > instance 2: memcpy(&dst->stats, &src->stats, sizeof(struct rtllib_rx_stats)); > > instance 3: memcpy(&dst->tim, &src->tim, sizeof(struct rtllib_tim_parameters)); > > instance 4: memcpy(dst->wzc_ie, src->wzc_ie, src->wzc_ie_len); > > > > There are a LOT more instances, here is the elixir link to that > > function for a quick reference: > > https://elixir.bootlin.com/linux/v5.11/source/drivers/staging/rtl8192e/rtllib_rx.c#L2420 > > > > My point is, it's clear that the intent of this function is to duplicate > > the data of src into dst. If "CcxRmState" really is supposed to be u16, > > then why only write down the first 2 bytes into "dst->CcxRmState"? > > > > What about "dst->CcxRmState[1]"? It never gets any value, again. > > > > These are the only places where CcxRmState is being used in the entire > > rtl8192e driver directory. I skipped line 1971 as it just checks whether > > "CcxRmState[0]" is 0 or not, this should not require any explanation. > > Ok, can you provide more information in the changelog text and resend > the commit so that it is easier to understand why you feel this change > is ok? Sure! Just a couple of doubts: 1. Do you want me to include all the information I wrote in my previous e-mail into the body of new e-mail? (Is there a limit to how long the body of the mail should be?) 2. I was asked to add the Fixes tag and also tag in -stable in my patch. I had a small confusion with the "Fixes" tag which I asked earlier in the thread but couldn't get it clarified with anyone, so I'll ask again: The previous commit pertaining to the lines I'm modifying is a checkpatch.pl fix (found using simple "git blame"). Should I write _that_ as the Fixes <Commit ID>? Or should I write in the commit id which created that file and hence, that specific line? (which makes more sense.) `git blame -L1960,1980 -- rtllib_rx.c` returns a single commit (80d2579d8608f) which was a checkpatch fix (1970, is the line my patch-1/1 modifies `git log -S'&info_element[4]' -- rtllib_rx.c` returned the commit (94a799425eee8) which created the file (the file which my patch-1/1 modifies) Which one should I write in the Fixes tag? Thanks for the review! Atul _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer 2021-02-20 18:21 ` Atul Gopinathan (?) (?) @ 2021-02-20 18:34 ` Gustavo A. R. Silva 2021-02-21 5:21 ` Atul Gopinathan -1 siblings, 1 reply; 19+ messages in thread From: Gustavo A. R. Silva @ 2021-02-20 18:34 UTC (permalink / raw) To: Atul Gopinathan, gregkh; +Cc: tiwai, devel, linux-kernel On 2/20/21 12:21, Atul Gopinathan wrote: > The variable "info_element" is of the following type: > struct rtllib_info_element *info_element > > rtllib_info_element is a struct containing the following fields as > defined in drivers/staging/rtl8192e/rtllib.h: > > struct rtllib_info_element { > u8 id; > u8 len; > u8 data[]; > } __packed; > > The following code of interest (to which this patch applies) is > supposed to check if the "info_element->len" is greater than 4 and > equal to 6, if this is satisfied then, the last two bytes (the > 4th and 5th index of u8 "data" array) are copied into > "network->CcxRmState". > > Currently the code uses "memcpy()" with the source as > "&info_element[4]" which would copy in wrong and unintended > information. > > This patch rectifies this error by using "&info_element->data[4]" which > rightly copies the last two bytes as the required state information. You should include a 'Fixes' tag for this. Also, is this code in -stable? If so, then tag this patch for stable, please. Thanks -- Gustavo > > Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> > --- > drivers/staging/rtl8192e/rtllib_rx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c > index 66c135321da4..15bbb63ca130 100644 > --- a/drivers/staging/rtl8192e/rtllib_rx.c > +++ b/drivers/staging/rtl8192e/rtllib_rx.c > @@ -1963,15 +1963,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; > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer 2021-02-20 18:34 ` [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer Gustavo A. R. Silva @ 2021-02-21 5:21 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-21 5:21 UTC (permalink / raw) To: Gustavo A. R. Silva, gregkh; +Cc: tiwai, devel, linux-kernel On Sat, Feb 20, 2021 at 12:34:15PM -0600, Gustavo A. R. Silva wrote: > > > On 2/20/21 12:21, Atul Gopinathan wrote: > > The variable "info_element" is of the following type: > > struct rtllib_info_element *info_element > > > > rtllib_info_element is a struct containing the following fields as > > defined in drivers/staging/rtl8192e/rtllib.h: > > > > struct rtllib_info_element { > > u8 id; > > u8 len; > > u8 data[]; > > } __packed; > > > > The following code of interest (to which this patch applies) is > > supposed to check if the "info_element->len" is greater than 4 and > > equal to 6, if this is satisfied then, the last two bytes (the > > 4th and 5th index of u8 "data" array) are copied into > > "network->CcxRmState". > > > > Currently the code uses "memcpy()" with the source as > > "&info_element[4]" which would copy in wrong and unintended > > information. > > > > This patch rectifies this error by using "&info_element->data[4]" which > > rightly copies the last two bytes as the required state information. > > You should include a 'Fixes' tag for this. Sure! Will resend the patches. I have a doubt about the Fixes tag, the previous commit pertaining to the lines I'm modifying is a checkpatch.pl fix (found using simple "git blame"). Should I write that as the Fixes <Commit ID>? Or should I write in the commit id which created that file and hence, that specific line? git blame -L1960,1980 -- rtllib_rx.c -> returns a single commit which was a checkpatch fix (1970, is the line my patch-1 modifies) git log -S'&info_element[4]' -- rtllib_rx.c -> returned the commit which created the file (the file which my patch-1 modifies) Which one should I write in the Fixes tag? Thanks! Atul ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer @ 2021-02-21 5:21 ` Atul Gopinathan 0 siblings, 0 replies; 19+ messages in thread From: Atul Gopinathan @ 2021-02-21 5:21 UTC (permalink / raw) To: Gustavo A. R. Silva, gregkh; +Cc: tiwai, devel, linux-kernel On Sat, Feb 20, 2021 at 12:34:15PM -0600, Gustavo A. R. Silva wrote: > > > On 2/20/21 12:21, Atul Gopinathan wrote: > > The variable "info_element" is of the following type: > > struct rtllib_info_element *info_element > > > > rtllib_info_element is a struct containing the following fields as > > defined in drivers/staging/rtl8192e/rtllib.h: > > > > struct rtllib_info_element { > > u8 id; > > u8 len; > > u8 data[]; > > } __packed; > > > > The following code of interest (to which this patch applies) is > > supposed to check if the "info_element->len" is greater than 4 and > > equal to 6, if this is satisfied then, the last two bytes (the > > 4th and 5th index of u8 "data" array) are copied into > > "network->CcxRmState". > > > > Currently the code uses "memcpy()" with the source as > > "&info_element[4]" which would copy in wrong and unintended > > information. > > > > This patch rectifies this error by using "&info_element->data[4]" which > > rightly copies the last two bytes as the required state information. > > You should include a 'Fixes' tag for this. Sure! Will resend the patches. I have a doubt about the Fixes tag, the previous commit pertaining to the lines I'm modifying is a checkpatch.pl fix (found using simple "git blame"). Should I write that as the Fixes <Commit ID>? Or should I write in the commit id which created that file and hence, that specific line? git blame -L1960,1980 -- rtllib_rx.c -> returns a single commit which was a checkpatch fix (1970, is the line my patch-1 modifies) git log -S'&info_element[4]' -- rtllib_rx.c -> returned the commit which created the file (the file which my patch-1 modifies) Which one should I write in the Fixes tag? Thanks! Atul _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-03-02 20:21 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-20 18:21 [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer Atul Gopinathan 2021-02-20 18:21 ` Atul Gopinathan 2021-02-20 18:21 ` [PATCH 2/2] staging: rtl8192e: Change state information from u16 to u8 Atul Gopinathan 2021-02-20 18:21 ` Atul Gopinathan 2021-02-21 13:08 ` Greg KH 2021-02-21 13:08 ` Greg KH 2021-02-21 16:57 ` Atul Gopinathan 2021-02-21 16:57 ` Atul Gopinathan 2021-02-22 15:26 ` Greg KH 2021-02-22 15:26 ` Greg KH 2021-02-22 17:23 ` Atul Gopinathan 2021-02-22 17:23 ` Atul Gopinathan 2021-03-02 14:38 ` Greg KH 2021-03-02 14:38 ` Greg KH 2021-03-02 16:53 ` Atul Gopinathan 2021-03-02 16:53 ` Atul Gopinathan 2021-02-20 18:34 ` [PATCH 1/2] staging: rtl8192e: Pass array value to memcpy instead of struct pointer Gustavo A. R. Silva 2021-02-21 5:21 ` Atul Gopinathan 2021-02-21 5:21 ` Atul Gopinathan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.