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  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:   Thu, 13 Aug 2020 12:46:05 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Qianli Zhao <zhaoqianligood@...il.com>, john.stultz@...aro.org,
        sboyd@...nel.org
Cc:     linux-kernel@...r.kernel.org, zhaoqianli@...omi.com
Subject: Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer

Qianli Zhao <zhaoqianligood@...il.com> writes:

Please start the first word after the colon with an upper case letter.

> do_init_timer can specify flags of timer_list,

Please write do_init_timer() so it's entirely clear that this is about a
function.

> but this function does not expect to specify the CPU or idx.

or idx does not mean anything unless someone looks at the
code. Changelogs want to explain things so they can be understood
without staring at the code.

> If user invoking do_init_timer and specify CPU,
> The result may not what we expected.

Right. And which caller exactly hands in crappy flags?

> E.g:
> do_init_timer invoked in core2,and specify flags 0x1
> final result flags is 0x3.If the specified CPU number
> exceeds the actual number,more serious problems will happen

More serious problems is not a really helpful technical explanation and
0x3 does not make sense for a changelog reader either because it again
requires to look up the code.

>  	timer->entry.pprev = NULL;
>  	timer->function = func;
> -	timer->flags = flags | raw_smp_processor_id();
> +	timer->flags = (flags & ~TIMER_BASEMASK & ~TIMER_ARRAYMASK) |
> raw_smp_processor_id();

If the caller hands in invalid flags then silently fixing them up is
fundamentally wrong. So this wants to be:

   if (WARN_ON(flags & ~TIMER_INIT_FLAGS))
   	flags &= TIMER_INIT_FLAGS;

and TIMER_INIT_FLAGS wants to be exactly the set of flags which are
valid for being handed in by a caller, i.e.:

      TIMER_DEFFERABLE, TIMER_PINNED, TIMER_IRQSAFE

Guess what happens when the caller hands in TIMER_MIGRATING?

If we do sanity checking then we do it correct and not just silently
papering over the particular problem which you ran into.

Thanks,

        tglx

Powered by blists - more mailing lists