Bug 105274 - Buffer overflow in gallium/auxiliary/hud/hud_cpufreq.c
Summary: Buffer overflow in gallium/auxiliary/hud/hud_cpufreq.c
Alias: None
Product: Mesa
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
Depends on:
Reported: 2018-02-27 18:47 UTC by vesim809
Modified: 2019-09-18 20:18 UTC (History)
0 users

See Also:
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)
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)

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


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.
Comment 4 GitLab Migration User 2019-09-18 20:18:20 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/922.

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.