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