Message ID | 151520105920.32271.1091443154687576996.stgit@dwillia2-desk3.amr.corp.intel.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Series | prevent bounds-check bypass via speculative execution | expand |
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 > > Based on an original patch by Elena Reshetova. > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > .../thermal/int340x_thermal/int340x_thermal_zone.c | 14 ++++++++ > ------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > index 145a5c53ff5c..442a1d9bf7ad 100644 > --- a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > +++ b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > @@ -17,6 +17,7 @@ > #include <linux/init.h> > #include <linux/acpi.h> > #include <linux/thermal.h> > +#include <linux/compiler.h> > #include "int340x_thermal_zone.h" > > static int int340x_thermal_get_zone_temp(struct thermal_zone_device > *zone, > @@ -52,20 +53,21 @@ static int int340x_thermal_get_trip_temp(struct > thermal_zone_device *zone, > int trip, int *temp) > { > struct int34x_thermal_zone *d = zone->devdata; > + unsigned long *elem; > int i; > > if (d->override_ops && d->override_ops->get_trip_temp) > return d->override_ops->get_trip_temp(zone, trip, > temp); > > - if (trip < d->aux_trip_nr) > - *temp = d->aux_trips[trip]; > - else if (trip == d->crt_trip_id) > + if ((elem = nospec_array_ptr(d->aux_trips, trip, d- > >aux_trip_nr))) { > + *temp = *elem; > + } else if (trip == d->crt_trip_id) { > *temp = d->crt_temp; > - else if (trip == d->psv_trip_id) > + } else if (trip == d->psv_trip_id) { > *temp = d->psv_temp; > - else if (trip == d->hot_trip_id) > + } else if (trip == d->hot_trip_id) { > *temp = d->hot_temp; > - else { > + } else { > for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; > i++) { > if (d->act_trips[i].valid && > d->act_trips[i].id == trip) { >
On Fri, Jan 5, 2018 at 5:53 PM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 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?
On 1/6/2018 4:10 AM, 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'. > > Based on an original patch by Elena Reshetova. > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > .../thermal/int340x_thermal/int340x_thermal_zone.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > index 145a5c53ff5c..442a1d9bf7ad 100644 > --- a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c > +++ b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c [...] > @@ -52,20 +53,21 @@ static int int340x_thermal_get_trip_temp(struct thermal_zone_device *zone, > int trip, int *temp) > { > struct int34x_thermal_zone *d = zone->devdata; > + unsigned long *elem; > int i; > > if (d->override_ops && d->override_ops->get_trip_temp) > return d->override_ops->get_trip_temp(zone, trip, temp); > > - if (trip < d->aux_trip_nr) > - *temp = d->aux_trips[trip]; > - else if (trip == d->crt_trip_id) > + if ((elem = nospec_array_ptr(d->aux_trips, trip, d->aux_trip_nr))) { And here... > + *temp = *elem; > + } else if (trip == d->crt_trip_id) { > *temp = d->crt_temp; > - else if (trip == d->psv_trip_id) > + } else if (trip == d->psv_trip_id) { > *temp = d->psv_temp; > - else if (trip == d->hot_trip_id) > + } else if (trip == d->hot_trip_id) { > *temp = d->hot_temp; > - else { > + } else { > for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) { > if (d->act_trips[i].valid && > d->act_trips[i].id == trip) { MBR, Sergei
On Fri, 2018-01-05 at 17:57 -0800, Dan Williams wrote: > On Fri, Jan 5, 2018 at 5:53 PM, Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> 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
diff --git a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c index 145a5c53ff5c..442a1d9bf7ad 100644 --- a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c +++ b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c @@ -17,6 +17,7 @@ #include <linux/init.h> #include <linux/acpi.h> #include <linux/thermal.h> +#include <linux/compiler.h> #include "int340x_thermal_zone.h" static int int340x_thermal_get_zone_temp(struct thermal_zone_device *zone, @@ -52,20 +53,21 @@ static int int340x_thermal_get_trip_temp(struct thermal_zone_device *zone, int trip, int *temp) { struct int34x_thermal_zone *d = zone->devdata; + unsigned long *elem; int i; if (d->override_ops && d->override_ops->get_trip_temp) return d->override_ops->get_trip_temp(zone, trip, temp); - if (trip < d->aux_trip_nr) - *temp = d->aux_trips[trip]; - else if (trip == d->crt_trip_id) + if ((elem = nospec_array_ptr(d->aux_trips, trip, d->aux_trip_nr))) { + *temp = *elem; + } else if (trip == d->crt_trip_id) { *temp = d->crt_temp; - else if (trip == d->psv_trip_id) + } else if (trip == d->psv_trip_id) { *temp = d->psv_temp; - else if (trip == d->hot_trip_id) + } else if (trip == d->hot_trip_id) { *temp = d->hot_temp; - else { + } else { for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) { if (d->act_trips[i].valid && d->act_trips[i].id == trip) {