Closed AmosChenYQ closed 2 years ago
reallocarray is implemented in glibc 2.26 (https://github.com/bminor/glibc/commit/2e0bbbfbf95fc9e22692e93658a6fbdd2d4554da), but Ubuntu Xenial uses 2.23 (https://launchpad.net/ubuntu/xenial/+source/glibc)
We could polyfill this with gnulib (https://www.gnu.org/software/gnulib/MODULES.html#module=reallocarray) or something, but is it really worth the effort to support an already EOL-ed system?
Thanks for replying, I have built and installed nvtop from an older version without the need of reallocarray. For the polyfill method, I will have a try.
Indeed, reallocarray came from BSD and has been introduced "recently" in the glibc and even more recently in other libc (e.g. 2021 for musl libc). I'll add a check with CMake to see if the function exists, and otherwise use realloc. This should fix your compilation issue.
@AmosChenYQ Could you please try if 816c34894b194d4ff5c1bcb2a6d3f366c1a78934 works for you? To do so,
cd nvtop
git fetch origin
git checkout reallocarrayFix
and follow the build process in the README
#define reallocarray(ptr, nbmem, size) realloc(ptr, (nbmem) * (size))
The difference between a reallocarray and a realloc with a multiplication is that reallocarray will refuse to allocate when the multiplication overflows, which, if it happens, can lead to memory corruptions and potentially vulnerabilities.
My calculation shows that for our use cases, this isn't really exploitable. It would most likely overflow on a 32 bit arch, and we use it on two locations:
seen_fds = reallocarray(seen_fds, seen_fds_capacity, sizeof(*seen_fds));
and
*processes_info = reallocarray(*processes_info, processes_info_capacity, sizeof(**processes_info));
For seen_fds sizeof(*seen_fds)
= 4, so the capacity would be at least (1<<32)/4
= 1073741824. The capacity increment are 1 below each power of two:
>>> 0
0
>>> _*2 + 1
1
>>> _*2 + 1
3
>>> _*2 + 1
7
>>> _*2 + 1
15
>>> _*2 + 1
31
So one would need at least 1073741824 fds on a single process pointing to different in-kernel struct file
. That is impossible on 32 bit arch with the memory limitations.
For the latter case, I think sizeof(**processes_info)
would be 64 (didn't compile, just doing this on paper):
/* offset | size */ type = struct gpu_process {
/* 0 | 4 */ enum gpu_process_type type;
/* 4 | 4 */ pid_t pid;
/* 8 | 4 */ char *cmdline;
/* 12 | 4 */ char *user_name;
/* 16 | 4 */ unsigned int gpu_usage;
/* 20 | 4 */ unsigned int encode_usage;
/* 24 | 4 */ unsigned int decode_usage;
/* XXX 4-byte hole */
/* 32 | 8 */ unsigned long long gpu_memory_usage;
/* 40 | 4 */ unsigned int gpu_memory_percentage;
/* 44 | 4 */ unsigned int cpu_usage;
/* 48 | 4 */ unsigned long cpu_memory_virt;
/* 52 | 4 */ unsigned long cpu_memory_res;
/* 56 | 2 */ unsigned char valid[2];
/* XXX 6-byte padding */
/* total size (bytes): 64 */
}
So to overflow this you need (1<<32)/64
= 67108864 PIDs (assuming the worst case scenario where an adversary is racing so that PIDs don't have to coexist at the same time). This is currently impossible but may possibly extendable in the future (per threads.h max PID is 32768 on 32bit, 4 million on 64 bit, but theoretical limit is 1 billion).
Do you think it would make sense to say, add a comment there stating that it's not currently exploitable but must be careful with any future use of reallocarray this way?
@zhuyifei1999 Thanks for the analysis. And this is without saying that the OS will have to handle that many processes in the first place! I took a different approach and pushed a different patch, re-implementing the function altogether.
Solution merged in master
@AmosChenYQ Could you please try if 816c348 works for you? To do so,
cd nvtop git fetch origin git checkout reallocarrayFix
and follow the build process in the README
Sorry for the late response, I have replaced and built it successfully which I saw you have merged into main branch.
Distributor ID: Ubuntu Description: Ubuntu 16.04.7 LTS Release: 16.04 Codename: xenial
I tried to build nvtop from source but got these error messages
I searched for this
reallocarray
function, it is just a Standard C Library(libc, -lc) I don't think it can be missing. So I think it is because of lib link problemHere below is the makefile generated by cmake: