Bug 105274

Summary: Buffer overflow in gallium/auxiliary/hud/hud_cpufreq.c
Product: Mesa Reporter: vesim809
Component: OtherAssignee: mesa-dev
Status: NEW --- QA Contact: mesa-dev
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description vesim809 2018-02-27 18:47:48 UTC
It is possible to overflow cpufreq_info::name and cpufreq_info::sysfs_filename inside add_object function. It require to somehow(ex. inside chroot) create custom directory inside `/sys/devices/system/cpu/` named like cpu0<custom_data_here>` and it will pass: 
      if (sscanf(dp->d_name, "cpu%d\n", &cpu_index) != 1)
         continue;
inside hud_get_num_cpufreq function.
Comment 1 Gert Wollny 2018-02-27 19:38:54 UTC
cpufreq_info::sysfs_filename is a buffer of char[128] and the string fn is 
of the same size and it is written to by using snprintf, indicating its size, so unless I miss something the buffer overflow is only possible for cpufreq_info::name.
Comment 2 vesim809 2018-02-27 21:11:32 UTC
It is possible in both, you can create directory named "cpu0<very_long_string>" and it will be accepted by that sscanf. 
The easiest fix for it is by using strcpy_s instead of strcpy.
Comment 3 Gert Wollny 2018-02-27 23:00:39 UTC
The sscanf will accept the number and not check the rest of the string, yes, but later in the code you have

  char basename[256];
  snprintf(basename, sizeof(basename), 
           "/sys/devices/system/cpu/%s", dp->d_name);

so basename is limited to 256 chars including the terminator 0, and then 
  
  snprintf(fn, sizeof(fn), "%s/cpufreq/scaling_cur_freq", basename);

with sizeof(fn)== 128, which means fn is not longer then 128 byte, just like it is defined in cpufreq_info::sysfs_filename, hence no buffer overflow there. Then 

      if (stat(fn, &stat_buf) < 0)
         continue;

must pass, which means whatever string was created before, it must still point to a valid file name that at this point is

  /sys/devices/system/cpu/??/cpufreq/scaling_cur_freq

and this limits the size of ?? == dp->d_name to less than 80 characters (This, by the way, also means one has to create this special file, it is not sufficient to add an arbitrary string to  /sys/devices/system/cpu/cpuN). So yes, one can get a buffer overflow in cpufreq_info::name but it can not overwrite anything important, because given the memory layout of cpufreq_info it may temporarily overwrite cpufreq_info::cpu_index and a part of cpufreq_info::sysfs_filename, but these are set later in add_object anyway, and because of cpufreq_info::sysfs_filename being a null terminated string, cpufreq_info::name would also always be null-termianted, limiting the impact of its later use.

IMHO the better fix is snprintf, because it creates a null terminated string and doesn't do any post-hoc error handling. Unless someone beats me to it I'll send a patch tomorrow.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.