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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
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
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
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
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