Race condition in the i2c driver (IDFGH-14704)

[gomatteo]

Idf version: v5.4-561-gbcb3c32d3a

Hardware: ESP32-D0WD (revision v1.0)

There is a race condition in the i2c driver in case of timeout.
The problem lies here:

esp-idf/components/esp_driver_i2c/i2c_master.c

Lines 466 to 475 in bcb3c32

	while (i2c_master->i2c_trans.cmd_count) {
	if (xSemaphoreTake(i2c_master->cmd_semphr, ticks_to_wait) != pdTRUE) {
	// Software timeout, clear the command link and finish this transaction.
	i2c_master->cmd_idx = 0;
	i2c_master->trans_idx = 0;
	atomic_store(&i2c_master->status, I2C_STATUS_TIMEOUT);
	ESP_LOGE(TAG, "I2C software timeout");
	xSemaphoreGive(i2c_master->cmd_semphr);
	return;
	}

If an interrupt fires between line 470 and 471 then i2c_master->trans_idx is zeroed before changing i2c_master->status.
Inside the ISR the branch i2c_master->status == I2C_STATUS_READ is taken, leading to a null pointer dereference on line 650 (i2c_operation->data is null).

esp-idf/components/esp_driver_i2c/i2c_master.c

Lines 647 to 650 in bcb3c32

	if (i2c_master->status == I2C_STATUS_READ) {
	i2c_operation_t *i2c_operation = &i2c_master->i2c_trans.ops[i2c_master->trans_idx];
	portENTER_CRITICAL_ISR(&i2c_master->base->spinlock);
	i2c_ll_read_rxfifo(hal->dev, i2c_operation->data + i2c_operation->bytes_used, i2c_master->rx_cnt);

Here is a backtrace showing the crash

#0  0x40087cb4 in i2c_ll_read_rxfifo (hw=0x3ff53000, ptr=0x0, len=3 '\003') at /opt/esp/idf/components/hal/esp32/include/hal/i2c_ll.h:580
#1  i2c_isr_receive_handler (i2c_master=0x3ffd2e2c) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:650
#2  0x40087df1 in i2c_master_isr_handler_default (arg=0x3ffd2e2c) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:708
#3  0x40083a6c in shared_intr_isr (arg=0x3ffd336c) at /opt/esp/idf/components/esp_hw_support/intr_alloc.c:445
#4  0x40084e88 in _xt_lowint1 () at /opt/esp/idf/components/xtensa/xtensa_vectors.S:1240
#5  0x4011530e in s_i2c_send_commands (i2c_master=0x0, ticks_to_wait=0) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:471
#6  0x40115627 in s_i2c_transaction_start (i2c_dev=0x3ffd33b8, xfer_timeout_ms=<optimized out>) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:625
#7  0x40115727 in s_i2c_synchronous_transaction (i2c_dev=0x3ffd33b8, i2c_ops=0x3ffd29ec, ops_dim=4, timeout_ms=10) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:918
#8  0x4011673f in i2c_master_receive (i2c_dev=0x3ffd33b8, read_buffer=<optimized out>, read_size=4, xfer_timeout_ms=10) at /opt/esp/idf/components/esp_driver_i2c/i2c_master.c:1234
#9  0x400ec57e in HDC1080_ReadTempRhLow (f_Temp=0x3ffb5ed8 <PicoSensori+8>, f_Umidita=0x3ffb5edc <PicoSensori+12>) at /workspaces/pico_2023/main/HdcXX80.c:270
#10 0x400ec8b6 in GestioneHDCxx80 (StatusHDCxx80=0x3ffb5ed0 <PicoSensori>) at /workspaces/pico_2023/main/HdcXX80.c:141
#11 0x400e3c06 in task_hwlow (args=0x0) at /workspaces/pico_2023/main/main.c:1674
#12 0x4008f175 in vPortTaskWrapper (pxCode=0x400e3b2c <task_hwlow>, pvParameters=0x0) at /opt/esp/idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c:139

A possible solution is to set i2c_master->status to I2C_STATUS_TIMEOUT before zeroing i2c_master->trans_idx and i2c_master->cmd_idx.

Is there a reason why i2c_master->status is not read atomically inside the isr (on line 647)? Wouldn't it be better to use atomic_load instead?