linux-kernel - Re: [PATCH v2] staging: greybus: Convert timers to use timer

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]

Date:   Mon, 30 Oct 2017 12:35:58 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Bryan O'Donoghue <pure.logic@...us-software.ie>
Cc:     Kees Cook <keescook@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>,
        Alex Elder <elder@...nel.org>, greybus-dev@...ts.linaro.org,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[ Resend to Bryan's nexus-software address ]

On Tue, Oct 24, 2017 at 04:54:59PM +0100, Bryan O'Donoghue wrote:
> On 24/10/17 15:49, Kees Cook wrote:
> > In preparation for unconditionally passing the struct timer_list pointer to
> > all timer callbacks, switch to using the new timer_setup() and from_timer()
> > to pass the timer pointer explicitly.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: "Bryan O'Donoghue" <pure.logic@...us-software.ie>
> > Cc: Johan Hovold <johan@...nel.org>
> > Cc: Alex Elder <elder@...nel.org>
> > Cc: greybus-dev@...ts.linaro.org
> > Cc: devel@...verdev.osuosl.org
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > v2: Added back "get" in timer code, thanks to Bryan. :)
> > ---
> >   drivers/staging/greybus/loopback.c  | 19 +++++++++----------
> >   drivers/staging/greybus/operation.c |  7 +++----
> >   2 files changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 08e255884206..1c0bafeb7ea5 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work)
> >   	gb_loopback_async_operation_put(op_async);
> >   }
> >   
> > -static void gb_loopback_async_operation_timeout(unsigned long data)
> > +static void gb_loopback_async_operation_timeout(struct timer_list *t)
> >   {
> > -	struct gb_loopback_async_operation *op_async;
> > -	u16 id = data;
> > +	struct gb_loopback_async_operation *op_async =
> > +		from_timer(op_async, t, timer);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&gb_dev.lock, flags);
> > +	gb_loopback_async_operation_get(op_async);
> > +	spin_unlock_irqrestore(&gb_dev.lock, flags);
> 
> Damnit I'm just wrong (I hate that).
>
> The pointer can already have gone away by the time the timer runs - my 
> bad...

Hmm. Then something is really broken in this driver, you obviously must
never free the async operation which contains the timer while the timer
is active.

> see attached for update - with my Signed-off added.

The right thing to do here is to respin your patch from last year which
converts the loopback driver to use the timeout handling in greybus
core. Otherwise, I'm afraid you're not addressing the underlying bug.

Either way, Kees, please submit the operation.c conversion separately
from the loopback one, as the latter is non-trivial.

Thanks,
Johan