ColinIanKing / stress-ng

This is the stress-ng upstream project git repository. stress-ng will stress test a computer system in various selectable ways. It was designed to exercise various physical subsystems of a computer as well as the various operating system kernel interfaces.
https://github.com/ColinIanKing/stress-ng
GNU General Public License v2.0
1.82k stars 290 forks source link

stress_system_read can write outside of buffer #368

Closed estahlman-intel closed 8 months ago

estahlman-intel commented 8 months ago

It is possible for reads from the /sys filesystem to rarely return -EBUSY.

When that happens, stress_system_read() will write to a stack location outside the buffer

        ret = read(fd, buf, buf_len);
        if (UNLIKELY(ret < 0)) {
                buf[0] = '\0';
                ret = -errno;   // SET 
        }
        (void)close(fd);
        if ((ssize_t)buf_len == ret)
                buf[buf_len - 1] = '\0';
        else
                buf[ret] = '\0'; // USE

The ret value is set to -EBUSY at SET and then used in the USE statement, which will be before the buffer in the stack of the caller.

The corresponding strace shows this happening:

openat(AT_FDCWD, "/sys/devices/system/cpu/cpu123/cpufreq/scaling_max_freq", O_RDONLY) = 4
read(4, 0x7ffdea07e320, 128)            = -1 EBUSY (Device or resource busy)
close(4)                                = 0
--- SIGSEGV {si_signo=SIGSEGV, si_code=SI_KERNEL, si_addr=NULL} ---
+++ killed by SIGSEGV (core dumped) +++

Since the buffer is already set to zero at the beginning with memset will already have cleared the buffer, there's no need for the `else`` conditional.

ColinIanKing commented 8 months ago

Good catch! Thanks for the fix, I added a bit more to the commit message. Applied and pushed.