From: walter harms <wharms@bfs•de>
To: Firo Yang <firogm@gmail•com>
Cc: linux-hams@vger•kernel.org, netdev@vger•kernel.org
Subject: Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
Date: Wed, 17 May 2017 18:08:11 +0200 [thread overview]
Message-ID: <591C756B.8060109@bfs.de> (raw)
In-Reply-To: <20170517134246.GA24874@snow>
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
>>
next prev parent reply other threads:[~2017-05-17 16:08 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2017-05-18 3:29 ` Firo Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=591C756B.8060109@bfs.de \
--to=wharms@bfs$(echo .)de \
--cc=firogm@gmail$(echo .)com \
--cc=linux-hams@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox