linux-kernel - Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5j+ynuB+gCfyGXe35x0Pg40A6U9qbKCB=BPMCir92KiTVw@mail.gmail.com>
Date:   Fri, 1 Sep 2017 10:53:37 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Ian Abbott <abbotti@....co.uk>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        H Hartley Sweeten <hsweeten@...ionengravers.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

On Fri, Sep 1, 2017 at 2:38 AM, Ian Abbott <abbotti@....co.uk> 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@....co.uk>
>>> Cc: H Hartley Sweeten <hsweeten@...ionengravers.com>
>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>> Cc: devel@...verdev.osuosl.org
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>>   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