Messages in this thread | | | Date | Mon, 21 Feb 2022 15:22:50 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 2/3] staging: r8188eu: refactor rtw_ch2freq() |
| |
On Sun, Feb 20, 2022 at 05:30:08PM +0100, Michael Straube wrote: > On 2/20/22 17:20, Pavel Skripkin wrote: > > Hi Michael, > > > > On 2/20/22 18:48, Michael Straube wrote: > > > -static int ch_freq_map_num = ARRAY_SIZE(ch_freq_map); > > > - > > > u32 rtw_ch2freq(u32 channel) > > > { > > > - u8 i; > > > - u32 freq = 0; > > > - > > > - for (i = 0; i < ch_freq_map_num; i++) { > > > - if (channel == ch_freq_map[i].channel) { > > > - freq = ch_freq_map[i].frequency; > > > - break; > > > - } > > > - } > > > - if (i == ch_freq_map_num) > > > - freq = 2412; > > > - > > > - return freq; > > > + return ch_freq_map[channel - 1]; > > > } > > > > What if channel has wrong value? The old code returned some default > > value, but with new one we will hit OOB. > > > > Hi Pavel, > > thanks for reviewing. Yeah, I thought about adding a check for channel > value between 1 and 14. But I did not add it because I think if this > function will ever be called with channel < 1 or channel > 14, then the > calling code must be wrong. > > Would be nice to see what others think about this.
I'm glad that Pavel noticed this change. This is a risky thing and should have been noted in the commit message.
Just from a review stand point it would be best to leave the original behavior.
I have audited this change and I do not think it is safe. It seems to me that one way this can be controlled is via module_param(rtw_channel, int, 0644); in drivers/staging/r8188eu/os_dep/os_intfs.c. I don't see any checking on that.
regards, dan carpenter
| |