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.
>
>>
>> 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.
re,
wh
> Thanks,
> Firo
>
>>
>>
>> just my 2 cents,
>> re,
>> wh
>>
