Re: [PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
On Fri, Mar 5, 2021 at 12:33 PM Dan Carpenter wrote:
>
> It's good that you're reviewing code...
Right now watching the patches flow feels like I'm just shadowing.
Later, when I get the hang of it, I might try providing something on
my own. I'll just watch things from a distance for the time being
perhaps making questions here or there (like I just did).
Just in case, my main point was to use a min() (or MIN? whatever way
it's provided in the standard) to have only two conditions instead of
three... .to keep them on separate lines, it could be done like this:
if (sec_len > 0 &&
sec_len <= min(len, 32)) {
_but_ I understand if you would rather keep the 3 conditions.
Thanks for your comment.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
On Fri, Mar 05, 2021 at 10:58:17AM -0600, Edmundo Carmona Antoranz wrote:
> On Fri, Mar 5, 2021 at 2:59 AM Dan Carpenter wrote:
> > - if (sec_len > 0 && sec_len <= len) {
> > + if (sec_len > 0 &&
> > + sec_len <= len &&
> > + sec_len <= 32) {
>
> I wonder if this could be reduced to (sec_len > 0 && sec_len <=
> min(len, 32)) from a stylistic POV?
I kind of prefer it the way I wrote it. I prefer conditions split
apart and done ploddingly, one at a time... You'll notice how I could
have written it like:
if (sec_len > 0 && sec_len <= len &&
sec_len <= 32) {
But I really like my conditions to be spelled out so the "sec_len" is
perfectly aligned in each part of the condition. Your way would be to
combine two conditions into one part of a line and seems sneaky.
>
> First attempt at something kernel related so I know there's plenty of
> things to learn (including patterns for problems like this and
> etiquette).
It's good that you're reviewing code... We try to be predictable though
and no one would have predicted your response. Ideally patch review
should be like, "Ugh! Why didn't I think of that? Of course, we should
propagate the error code." Or "Oh, I didn't know checkpatch warns about
that."
The truth is that I don't always agree with all of Greg's reviews. He
is more strict than I am about breaking up patches into multiple things.
(It's a tricky line to define for me). But I can always predict what
Greg will say in a review so that saves time when I know which patches
he will accept and which he won't.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
On Fri, Mar 5, 2021 at 2:59 AM Dan Carpenter wrote:
> - if (sec_len > 0 && sec_len <= len) {
> + if (sec_len > 0 &&
> + sec_len <= len &&
> + sec_len <= 32) {
I wonder if this could be reduced to (sec_len > 0 && sec_len <=
min(len, 32)) from a stylistic POV?
First attempt at something kernel related so I know there's plenty of
things to learn (including patterns for problems like this and
etiquette).
BR
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()
This code has a check to prevent read overflow but it needs another
check to prevent writing beyond the end of the ->ssid[] array.
Fixes: a2c60d42d97c ("staging: r8188eu: Add files for new driver - part 16")
Signed-off-by: Dan Carpenter
---
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index bf22f130d3e1..58954b88a817 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -1133,9 +1133,11 @@ static int rtw_wx_set_scan(struct net_device *dev,
struct iw_request_info *a,
break;
}
sec_len = *(pos++); len -= 1;
- if (sec_len > 0 && sec_len <= len) {
+ if (sec_len > 0 &&
+ sec_len <= len &&
+ sec_len <= 32) {
ssid[ssid_index].ssid_length =
sec_len;
- memcpy(ssid[ssid_index].ssid,
pos, ssid[ssid_index].ssid_length);
+ memcpy(ssid[ssid_index].ssid,
pos, sec_len);
ssid_index++;
}
pos += sec_len;
--
2.30.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
