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.
Thanks,
Michael
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
On 2/21/22 13:22, Dan Carpenter wrote:
> 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.
>
Do you mean to leave the whole original code including the 5 GHz
frequencies? Or returning a default value if we have a channel value < 1
or > 14?
I'm a bit confused now, because Greg asked how we know that the driver
is only for 2.4 GHz chips.
> 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.
>
Thank you Dan!
I missed that and blindly assumed the function will never be called
with channel values OOB. That was not good, sorry.
regards,
Michael
Hi Michael,
On 2/21/22 22:20, Michael Straube wrote:
>> 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.
>>
>
> Do you mean to leave the whole original code including the 5 GHz
> frequencies? Or returning a default value if we have a channel value < 1
> or > 14?
>
IMO, your version is much cleaner than previous one. This table walk
seems really unreasonable, since 5 GHz support is really redundant (I
saw it in other thread)
I'd put just sanity check and return the default value from previous
version. Maybe even wrapped with unlikely() if we sure, that in normal
state we won't hit it ;)
With regards,
Pavel Skripkin
On Mon, Feb 21, 2022 at 08:20:14PM +0100, Michael Straube wrote:
> On 2/21/22 13:22, Dan Carpenter wrote:
> > On Sun, Feb 20, 2022 at 05:30:08PM +0100, Michael Straube wrote:
> > > On 2/20/22 17:20, Pavel Skripkin wrote:
> > Just from a review stand point it would be best to leave the original
> > behavior.
> >
>
> Do you mean to leave the whole original code including the 5 GHz
> frequencies? Or returning a default value if we have a channel value < 1
> or > 14?
I meant what Pavel said:
if (channel == 0 || channel > ARRAY_SIZE(ch_freq_map))
return 2412;
return ch_freq_map[channel - 1];
>
> I'm a bit confused now, because Greg asked how we know that the driver
> is only for 2.4 GHz chips.
>
Haven't you been deleting all the 5GHz code for some time now? Greg
isn't saying you have to keep that if there is no reason. Just put the
reason in the commit message and resend.
regards,
dan carpenter
On Mon, Feb 21, 2022 at 11:54:50PM +0300, Pavel Skripkin wrote:
> Hi Michael,
>
> On 2/21/22 22:20, Michael Straube wrote:
> > > 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.
> > >
> >
> > Do you mean to leave the whole original code including the 5 GHz
> > frequencies? Or returning a default value if we have a channel value < 1
> > or > 14?
> >
>
> IMO, your version is much cleaner than previous one. This table walk seems
> really unreasonable, since 5 GHz support is really redundant (I saw it in
> other thread)
>
> I'd put just sanity check and return the default value from previous
> version. Maybe even wrapped with unlikely() if we sure, that in normal state
> we won't hit it ;)
Adding likely()/unlikely() annotations hurt readability. Do not do that
unless you have benchmark data to prove that it is worth it. (Hint: it
is not worth it here).
regards,
dan carpenter