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]
Message-ID: <87pltjj1bs.ffs@tglx>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ