Google Android max86902 Driver - 'sysfs' Interfaces Race Condition

Source: https://bugs.chromium.org/p/project-zero/issues/detail?id=963

The MAX86902 sensor has a driver that exposes several interfaces through which the device may be configured. In addition to exposing a character device, it also exposes several entries under sysfs.

Some of these entries are writable, allowing different values to be configured. Three such files are exposed under the paths:

/sys/devices/virtual/sensors/hrm_sensor/eol_test_result 
/sys/devices/virtual/sensors/hrm_sensor/lib_ver
/sys/devices/virtual/sensors/uv_sensor/uv_lib_ver

The sysfs write handlers for these files all share approximately the same logic. Below is one such handler, for the "uv_lib_ver" sysfs entry:

1.  static ssize_t max86900_uv_lib_ver_store(struct device *dev,
2.  	struct device_attribute *attr, const char *buf, size_t size)
3.  {
4.  	struct max86900_device_data *data = dev_get_drvdata(dev);
5.  	unsigned int buf_len;
6.  	buf_len = (unsigned int)strlen(buf) + 1;
7.  	if (buf_len > MAX_LIB_VER)
8.  		buf_len = MAX_LIB_VER;
9.  
10. 	if (data->uv_lib_ver != NULL)
11.		kfree(data->uv_lib_ver);
12.
13.	data->uv_lib_ver = kzalloc(sizeof(char) * buf_len, GFP_KERNEL);
14.	if (data->uv_lib_ver == NULL) {
15.		pr_err("%s - couldn't allocate memory\n", __func__);
16.		return -ENOMEM;
17.	}
18.	strncpy(data->uv_lib_ver, buf, buf_len);
19.	pr_info("%s - uv_lib_ver = %s\n", __func__, data->uv_lib_ver);
20.	return size;
21. }

Since the code above does not use any mechanism to prevent concurrent access, it contains race conditions which allow corruption of kernel memory.

For example, one such race condition could occur when two attempts to call "write" are executed at the same time, where the underlying buffers have different lengths. More concretely, denote the two accessing tasks "task1" and "task2", correspondingly. Consider the following sequence of events:

  -"task1" attempts to write to the entry, and provides a buffer of length 20. 
  -"task1" manages to execute lines 1-17 (inclusive)
  -"task2" now attempts to write to the entry, and provides a buffer of length 2.
  -"task2" manages to execute lines 1-13 (inclusive)
  -"task1" now executes line 18, resulting in an overflow when writing to data->uv_lib_ver (since its actual length is now 2)

This issue can be addressed by adequate locking when accessing the sysfs entries.

I've statically and dynamically verified this issue on an SM-G935F device. The open-source kernel package I analysed was "SM-G935F_MM_Opensource", the device's build is "XXS1APG3".

The sysfs entries mentioned above have UID "system" and GID "radio". The SELinux context for these entries is: "u:object_r:sysfs_sensor_writable:s0".

According to the default SELinux rules as present on the SM-G935F (version XXS1APG3), the following contexts may access these files:

 allow radio sysfs_sensor_writable : file { ioctl read write getattr lock append open } ; 
 allow factory_adsp sysfs_sensor_writable : file { ioctl read write getattr lock append open } ; 
 allow sensorhubservice sysfs_sensor_writable : file { write append open } ; 
 allow sysfs_sensor_writable sysfs_sensor_writable : filesystem associate ; 
 allow system_app sysfs_sensor_writable : file { ioctl read write getattr lock append open } ; 


Proof of Concept:
https://gitlab.com/exploit-database/exploitdb-bin-sploits/-/raw/main/bin-sploits/40993.zip