On Wed, May 17, 2017 at 06:08:11PM +0200, walter harms wrote:
>
>
>Am 17.05.2017 15:42, schrieb Firo Yang:
>> On Wed, May 17, 2017 at 02:59:39PM +0200, walter harms wrote:
>>>
>>>
>>> Am 17.05.2017 14:35, schrieb Firo Yang:
>>>> The divisor s->par.bitrate will always be 0 until initialized by
>>>> ndo_open() and hdlcdrv_open().
>>>>
>>>> In order to fix this divide zero error, check whether the netdevice
>>>> was opened by ndo_open() before performing divide.
>>>>
>>>> Reported-by: Dmitry Vyukov <dvyu...@google.com>
>>>> Signed-off-by: Firo Yang <fir...@gmail.com>
>>>> ---
>>>> drivers/net/hamradio/hdlcdrv.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/hamradio/hdlcdrv.c
>>>> b/drivers/net/hamradio/hdlcdrv.c
>>>> index 8c3633c..3c783fd 100644
>>>> --- a/drivers/net/hamradio/hdlcdrv.c
>>>> +++ b/drivers/net/hamradio/hdlcdrv.c
>>>> @@ -574,7 +574,7 @@ static int hdlcdrv_ioctl(struct net_device *dev,
>>>> struct ifreq *ifr, int cmd)
>>>> break;
>>>>
>>>> case HDLCDRVCTL_CALIBRATE:
>>>> - if(!capable(CAP_SYS_RAWIO))
>>>> + if (!capable(CAP_SYS_RAWIO) || !netif_running(dev))
>>>> return -EPERM;
>>>> if (bi.data.calibrate > INT_MAX / s->par.bitrate)
>>>> return -EINVAL;
>>>
>>> I would still check for s->par.bitrate > 0 later changes may affect the
>>> setting of it
>>> and it is much more obvious.
>>
>> I think 0 is not valid value for bitrate, so we should check it in
>> other places, like what ser12_open() did:
>> 429 if (bc->baud < 300 || bc->baud > 4800) {
>> 430 printk(KERN_INFO "baycom_ser_fdx: invalid baudrate "
>> 431 "(300...4800)\n");
>> 432 return -EINVAL;
>> 433 }
>> ...
>> 440 bc->hdrv.par.bitrate = bc->baud;
>
>
>I do not want to say you change is not valid but i have learned that it is
>better to
>have an obvious check that to rely on hidden knowledge.
I agree with this.
>
>
>>
>>>
>>> Also perhaps !netif_running(dev) should better return ENODEV.
>>
>> However, the 'dev' truly exists in this circumstance.
>>
>
>yes and i do not feel good with that but "no permission" will lead
>any enduser into a search for user rights.
Indeed, ENODEV is more informative to enduser.
I will send a update patch.
Thanks,
Firo
>
>
>
>re,
> wh
>
>
>> Thanks,
>> Firo
>>
>>>
>>>
>>> just my 2 cents,
>>> re,
>>> wh
>>>
