* [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
@ 2017-05-17 12:35 Firo Yang
2017-05-17 12:59 ` walter harms
0 siblings, 1 reply; 5+ messages in thread
From: Firo Yang @ 2017-05-17 12:35 UTC (permalink / raw)
To: t.sailer; +Cc: davem, gregkh, linux-hams, netdev, dvyukov, syzkaller, 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 <dvyukov@google.com>
Signed-off-by: Firo Yang <firogm@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;
--
2.9.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
2017-05-17 12:35 [PATCH] hdlcdrv: fix divide error bug if bitrate is 0 Firo Yang
@ 2017-05-17 12:59 ` walter harms
2017-05-17 13:42 ` Firo Yang
0 siblings, 1 reply; 5+ messages in thread
From: walter harms @ 2017-05-17 12:59 UTC (permalink / raw)
To: Firo Yang; +Cc: t.sailer, davem, gregkh, linux-hams, netdev, dvyukov, syzkaller
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 <dvyukov@google.com>
> Signed-off-by: Firo Yang <firogm@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.
Also perhaps !netif_running(dev) should better return ENODEV.
just my 2 cents,
re,
wh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
2017-05-17 12:59 ` walter harms
@ 2017-05-17 13:42 ` Firo Yang
2017-05-17 16:08 ` walter harms
0 siblings, 1 reply; 5+ messages in thread
From: Firo Yang @ 2017-05-17 13:42 UTC (permalink / raw)
To: walter harms
Cc: t.sailer, davem, gregkh, linux-hams, netdev, dvyukov, syzkaller
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 <dvyukov@google.com>
>> Signed-off-by: Firo Yang <firogm@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;
>
>Also perhaps !netif_running(dev) should better return ENODEV.
However, the 'dev' truly exists in this circumstance.
Thanks,
Firo
>
>
>just my 2 cents,
>re,
> wh
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
2017-05-17 13:42 ` Firo Yang
@ 2017-05-17 16:08 ` walter harms
2017-05-18 3:29 ` Firo Yang
0 siblings, 1 reply; 5+ messages in thread
From: walter harms @ 2017-05-17 16:08 UTC (permalink / raw)
To: Firo Yang; +Cc: linux-hams, netdev
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 <dvyukov@google.com>
>>> Signed-off-by: Firo Yang <firogm@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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
2017-05-17 16:08 ` walter harms
@ 2017-05-18 3:29 ` Firo Yang
0 siblings, 0 replies; 5+ messages in thread
From: Firo Yang @ 2017-05-18 3:29 UTC (permalink / raw)
To: walter harms; +Cc: linux-hams, netdev
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 <dvyukov@google.com>
>>>> Signed-off-by: Firo Yang <firogm@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
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-18 3:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-17 12:35 [PATCH] hdlcdrv: fix divide error bug if bitrate is 0 Firo Yang
2017-05-17 12:59 ` walter harms
2017-05-17 13:42 ` Firo Yang
2017-05-17 16:08 ` walter harms
2017-05-18 3:29 ` Firo Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).