Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: bRfx4nvFkBkTWlRyuZpNn31ylIAG/Cb9kg5DjNLxKFesEv8/J2JS3pIbICOh8Ptmi+FVeiIUBx
 l2XOvPR5zbPQ==
IronPort-SDR: 4v+838BTvv5dSQJgqNNoDW1HyJDXWviBpViFsaEAdbPvFCcyfksE/qNkxfGGJhuEHsPThYg4yT
 g8ByHB4Aatuw==
From:   "Zhang, Rui" <rui.zhang@intel.com>
To:     Takashi Iwai <tiwai@suse.de>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
CC:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Len Brown <lenb@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: OOB access on ACPI processor thermal device via sysfs write
Thread-Topic: OOB access on ACPI processor thermal device via sysfs write
Thread-Index: AQHWCMInBN92pltZsEqESgUJHFcCaKhlcwvw
Date:   Thu, 2 Apr 2020 07:47:50 +0000
Message-ID: <744357E9AAD1214791ACBA4B0B90926377CEB399@SHSMSX108.ccr.corp.intel.com>
References: <s5h5zeiwd01.wl-tiwai@suse.de>
In-Reply-To: <s5h5zeiwd01.wl-tiwai@suse.de>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.2.0.6
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

CC Viresh.

Yes, I've received it.

To me, there is not a hard rule that the cooling device max_state must be static.
We should be able to detect the max_state change and reset the stats table when necessary.

I just finished a prototype patch to do so, and will paste it later.

Thanks,
rui

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Thursday, April 02, 2020 3:42 PM
To: linux-acpi@vger.kernel.org
Cc: Zhang, Rui <rui.zhang@intel.com>; Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; linux-kernel@vger.kernel.org
Subject: OOB access on ACPI processor thermal device via sysfs write
Importance: High

[ My post yesterday didn't seem go out properly, so I'm resending with
  a different MTA setup; sorry if you already received it ]

Hi,

after my recent patch [*], I started looking at the root cause more closely, and found that it's a side-effect of the device initialization order between acpi_processor_driver and cpufreq.
  [*] https://lore.kernel.org/linux-pm/20200330140859.12535-1-tiwai@suse.de/

In short:
  
- acpi_processor_driver_init() is called fairly early boot stage, and
  it registers a thermal device for each CPU. 

- A thermal device allocates a statistics table with the fixed size
  determined by get_max_state() ops call at its probe time.
  (The table entry is updated at each time a write to thermal state
  file happens.)

  For ACPI, processor_get_max_state() is called and it invokes
  acpi_processor_max_state() that calculates the max states depending
  on cpufreq, cpufreq_get_max_state().

  cpufreq_get_max_state() returns 0 at this stage because no cpufreq
  driver is available.  So the table size is set to 1.

- Later on, after cpufreq get initialized, the following sysfs write
  causes an OOB access and corrupts memory (eventually Oops):
    # echo 3 > /sys/class/thermal/cooling_device0/cur_state

  The reason is that now processor_get_max_state() returns 3 as
  cpufreq became available.  So the thermal driver believes as if it
  were a valid value, and updates the table accordingly although it
  overflows.


My patch above detects and avoids such an overflow, but it's no real fix for the cause.  The proper fix needs either the shuffling of the device creation order or some automatic resize of statistics table.

Do you guys have any suggestion how to solve it?

FWIW, I could reproduce the problem locally on my laptop with 5.6 kernel, and I believe this can be seen generically on most of machines.


thanks,

Takashi