linux-hardening - Re: [PATCH v2] ntp: safeguard against time

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] [day] [month] [year] [list]

Date: Sat, 18 May 2024 04:33:43 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Justin Stitt <justinstitt@...gle.com>, John Stultz <jstultz@...gle.com>,
 Stephen Boyd <sboyd@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 Bill Wendling <morbo@...gle.com>
Cc: linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
 linux-hardening@...r.kernel.org, Justin Stitt <justinstitt@...gle.com>
Subject: Re: [PATCH v2] ntp: safeguard against time_constant overflow case

Justin!

On Fri, May 17 2024 at 00:47, Justin Stitt wrote:
>  	if (txc->modes & ADJ_TIMECONST) {
> -		time_constant = txc->constant;
> -		if (!(time_status & STA_NANO))
> +		if (!(time_status & STA_NANO) && time_constant < MAXTC)
>  			time_constant += 4;
>  		time_constant = min(time_constant, (long)MAXTC);
>  		time_constant = max(time_constant, 0l);

Let me digest this.

The original code does:

	time_constant = txc->constant;
	if (!(time_status & STA_NANO))
		time_constant += 4;
	time_constant = min(time_constant, (long)MAXTC);
	time_constant = max(time_constant, 0l);

Your change results in:

	if (!(time_status & STA_NANO) && time_constant < MAXTC)
		time_constant += 4;
	time_constant = min(time_constant, (long)MAXTC);
	time_constant = max(time_constant, 0l);

IOW, you lost the intent of the code to assign the user space supplied
value of txc->constant.

Aside of that you clearly failed to map the deep analysis I provided to
you vs. the time_maxerror issue to this one:

# git grep 'time_constant.*=' kernel/time/
ntp.c:66:static long                    time_constant = 2;

  That's the static initializer

kernel/time/ntp.c:736:              time_constant = txc->constant;
kernel/time/ntp.c:738:                      time_constant += 4;
kernel/time/ntp.c:739:              time_constant = min(time_constant, (long)MAXTC);
kernel/time/ntp.c:740:              time_constant = max(time_constant, 0l);

  That's the part of process_adjtimex_modes() you are trying to
  "fix". So it's exactly the same problem as with time_maxerror, no?

And therefore you provide a "safeguard" against overflow for the price of
making the syscall disfunctional. Seriously?

Did you even try to run something else than the bad case reproducer
against your fix?

No. You did not. Any of the related real use case tests would have
failed.

I told you yesterday:

   Tools are good to pin-point symptoms, but they are by definition
   patently bad in root cause analysis. Otherwise we could just let the
   tool write the "fix".

Such a tool would have at least produced a correct "fix" to cure the
symptom.

Thanks,

        tglx