linux-kernel - Re: [GIT PULL] time/ntp fix

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:	Fri, 20 Feb 2015 14:58:49 -0800
From:	John Stultz <john.stultz@...aro.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [GIT PULL] time/ntp fix

On Fri, Feb 20, 2015 at 2:26 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Fri, Feb 20, 2015 at 5:44 AM, Ingo Molnar <mingo@...nel.org> wrote:
>>
>> John Stultz (1):
>>       ntp: Fixup adjtimex freq validation on 32-bit systems
>
> This is confusing. 32-bit?

Right, so the check that was added in a previous commit is really only
a concern for 64bit systems, but was applied to both 32 and 64bit
systems, which results in breaking 32bit systems.

Thus the "fix" here is to make the check only apply to 64bit systems.

>> +       /*
>> +        * Check for potential multiplication overflows that can
>> +        * only happen on 64-bit systems:
>
> 64-bit?
>
>> +       if ((txc->modes & ADJ_FREQUENCY) && (BITS_PER_LONG == 64)) {
>
> Hmm. The code clearly says "&& (BITS_PER_LONG == 64)"
>
> But:
>
>> +               if (LLONG_MIN / PPM_SCALE > txc->freq)
>>                         return -EINVAL;
>> -               if (LONG_MAX / PPM_SCALE < txc->freq)
>> +               if (LLONG_MAX / PPM_SCALE < txc->freq)
>>                         return -EINVAL;
>
> The difference between LONG_MAX and LLONG_MAX only matters if
> BITS_PER_LONG would be 32.

Right. So v1 of this fix just changed the LONG_MIX/MAX to be
LLONG_MIN/MAX. This resolves the issue, but with some compiler checks
we get warnings since LLONG_MIN/MAX is always smaller/larger then a
32bit value. Thus the extra BITS_PER_LONG check (which I think is a
bit ugly, but I didn't get any better suggestions) was added to avoid
the build warnings as well.

So the fix is somewhat redundant, but I do think the LLONG specifier
is more exact.

> So the changes are confusing to begin with and the commit log
> description doesn't match them anyway.
>
> I'm not pulling this without clarifications.

Is the above clarification sufficient? Or would you like me to reword
the commit message as well?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/