Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753844AbeAFRYj (ORCPT + 1 other); Sat, 6 Jan 2018 12:24:39 -0500 Received: from mga11.intel.com ([192.55.52.93]:32629 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753680AbeAFRYh (ORCPT ); Sat, 6 Jan 2018 12:24:37 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,322,1511856000"; d="scan'208";a="21943424" Message-ID: <1515259475.23948.15.camel@linux.intel.com> Subject: Re: [PATCH 12/18] Thermal/int340x: prevent bounds-check bypass via speculative execution From: Srinivas Pandruvada To: Dan Williams Cc: Linux Kernel Mailing List , linux-arch@vger.kernel.org, Greg KH , Peter Zijlstra , Netdev , Eduardo Valentin , Zhang Rui , Linus Torvalds , Thomas Gleixner , Elena Reshetova , Alan Cox Date: Sat, 06 Jan 2018 09:24:35 -0800 In-Reply-To: References: <151520099201.32271.4677179499894422956.stgit@dwillia2-desk3.amr.corp.intel.com> <151520105920.32271.1091443154687576996.stgit@dwillia2-desk3.amr.corp.intel.com> <1515203635.26317.78.camel@linux.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2 (3.18.5.2-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Fri, 2018-01-05 at 17:57 -0800, Dan Williams wrote: > On Fri, Jan 5, 2018 at 5:53 PM, Srinivas Pandruvada > wrote: > > > > On Fri, 2018-01-05 at 17:10 -0800, Dan Williams wrote: > > > > > > Static analysis reports that 'trip' may be a user controlled > > > value > > > that > > > is used as a data dependency to read '*temp' from the 'd- > > > >aux_trips' > > > array.  In order to avoid potential leaks of kernel memory > > > values, > > > block > > > speculative execution of the instruction stream that could issue > > > reads > > > based on an invalid value of '*temp'. > > Not against the change as this is in a very slow path. But the trip > > is > > not an arbitrary value which user can enter. > > > > This trip value is the one of the sysfs attribute in thermal zone. > > For > > example > > > > # cd /sys/class/thermal/thermal_zone1 > > # ls trip_point_?_temp > > trip_point_0_temp  trip_point_1_temp  trip_point_2_temp  trip_point > > _3_t > > emp  trip_point_4_temp  trip_point_5_temp  trip_point_6_temp > > > > Here the "trip" is one of the above trip_point_*_temp. So in this > > case > > it can be from 0 to 6 as user can't do > > # cat trip_point_7_temp > > as there is no sysfs attribute for trip_point_7_temp. > > > > The actual "trip" was obtained in thermal core via > > > >       if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != > > 1) > >                 return -EINVAL; > > > > Thanks, > > Srinivas > Ah, great, thanks. So do we even need the bounds check at that point? We are not bound checking but the way we identify type of the trip. Based on ACPI support we order trips: - Aux trips max_count = aux_trip_nr - One Critical trip - One Hot trip - One passive trip - Rest all trips are active trips So in the above example if d->aux_trip_nr is 1 then trip_point_0_temp read/write is for aux trip. If d->aux_trip_nr is 0 then it can be any other non aux trip. BUT I am not still up to date with these attacks. Not sure about the perimeter of user controlled value. It is a user controlled but limited by the sysfs attributes. So I will test this patch and let you know if there are any issues. Thanks, Srinivas