On Fri, Sep 1, 2017 at 2:38 AM, Ian Abbott <abbotti@xxxxxxxxx> wrote:
> On 01/09/17 10:29, Ian Abbott wrote:
>>
>> On 01/09/17 00:29, Kees Cook wrote:
>>>
>>> With timer initialization made unconditional, there is no reason to
>>> make del_timer_sync() calls conditionally, there by removing the test
>>> of the .data field.
>>>
>>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>>> Cc: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: devel@xxxxxxxxxxxxxxxxxxxx
>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>> ---
>>> drivers/staging/comedi/drivers/das16.c | 9 +++------
>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/das16.c
>>> b/drivers/staging/comedi/drivers/das16.c
>>> index 5d157951f63f..4514179b2007 100644
>>> --- a/drivers/staging/comedi/drivers/das16.c
>>> +++ b/drivers/staging/comedi/drivers/das16.c
>>> @@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device
>>> *dev, unsigned int dma_chan)
>>> /* DMA uses two buffers */
>>> devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan,
>>> DAS16_DMA_SIZE, COMEDI_ISADMA_READ);
>>> - if (devpriv->dma) {
>>> - setup_timer(&devpriv->timer, das16_timer_interrupt,
>>> - (unsigned long)dev);
>>> - }
>>> + setup_timer(&devpriv->timer, das16_timer_interrupt,
>>> + (unsigned long)dev);
>>> }
>>
>>
>> das16_alloc_dma() returns before the call to comedi_isadma_alloc() if the
>> dma_chan parameter is not one of the values 1 or 3, so setup_timer() will
>> not be called in that case.
>>
>>> static void das16_free_dma(struct comedi_device *dev)
>>> @@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev)
>>> struct das16_private_struct *devpriv = dev->private;
>>> if (devpriv) {
>>> - if (devpriv->timer.data)
>>> - del_timer_sync(&devpriv->timer);
>>> + del_timer_sync(&devpriv->timer);
>>
>>
>> If setup_timer() has not been called (see remark above), this change will
>> break.
>>
>>> comedi_isadma_free(devpriv->dma);
>>> }
>>> }
>>>
>
> If you want to avoid testing devpriv->timer.data for some reason, you could
> make the calls to setup_timer() and del_timer_sync() depend on devpriv->dma.
Thanks for checking this! I think the cleanest would be to just move
setup_timer() to the start of das16_alloc_dma(). I'll make that
adjustment.
-Kees
--
Kees Cook
Pixel Security
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
