|Summary:||Buffer overflow in gallium/auxiliary/hud/hud_cpufreq.c|
|Status:||NEW ---||QA Contact:||mesa-dev|
|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 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; 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.