| Summary: | Buffer overflow in gallium/auxiliary/hud/hud_cpufreq.c | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | vesim809 |
| Component: | Other | Assignee: | mesa-dev |
| Status: | RESOLVED MOVED | QA Contact: | mesa-dev |
| Severity: | normal | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
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. 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. 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.
-- 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.
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.