SPI - STM32: transceive() should handle null tx buffer

[a8961713]

Describe the bug
According to the documentation for struct spi_buf in spi.h, when setting buf member to a null-pointer, dummy bytes should be sent on MOSI line.

/**
 * @brief SPI buffer structure
 *
 * @param buf is a valid pointer on a data buffer, or NULL otherwise.
 * @param len is the length of the buffer or, if buf is NULL, will be the
 *    length which as to be sent as dummy bytes (as TX buffer) or
 *    the length of bytes that should be skipped (as RX buffer).
 */
struct spi_buf {
	void *buf;
	size_t len;
};

But when using STM32 driver spi_ll_stm32.c, setting buf to a null pointer results in a null-pointer de-reference (e.g. line 100):

	if (SPI_WORD_SIZE_GET(data->ctx.config->operation) == 8) {
		if (spi_context_tx_on(&data->ctx)) {
			/****** HERE ******/ tx_frame = UNALIGNED_GET((u8_t *)(data->ctx.tx_buf));
		}
		LL_SPI_TransmitData8(spi, tx_frame);
		/* The update is ignored if TX is off. */
		spi_context_update_tx(&data->ctx, 1, 1);
	} else {
		if (spi_context_tx_on(&data->ctx)) {
			tx_frame = UNALIGNED_GET((u16_t *)(data->ctx.tx_buf));
		}
		LL_SPI_TransmitData16(spi, tx_frame);
		/* The update is ignored if TX is off. */
		spi_context_update_tx(&data->ctx, 2, 1);
	}

The cause for the problem stems from #20948 and later from #21772
These commits replaced the call to spi_context_tx_buf_on() with a call to spi_context_tx_on().
While spi_context_tx_buf_on() is checking for the validity of tx buf pointer, spi_context_tx_on() does not, and so a null-pointer de-reference occurs.

To Reproduce
A test case was added to the spi_loopback test under tests/drivers/spi/ which makes an SPI spi_transceive() call with a null tx buffer.
Here is a patch for this test case + a configuration file for running on the nucleo_f413zh board:

diff --git a/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/boards/nucleo_f413zh.conf b/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/boards/nucleo_f413zh.conf
new file mode 100644
index 00000000..2d9df89e
--- /dev/null
+++ b/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/boards/nucleo_f413zh.conf
@@ -0,0 +1,2 @@
+CONFIG_SPI_LOOPBACK_DRV_NAME="SPI_1"
+CONFIG_SPI_STM32_INTERRUPT=y
diff --git a/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/src/spi.c b/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/src/spi.c
index c94724bf..298c3530 100644
--- a/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/src/spi.c
+++ b/zephyrproject/zephyr/tests/drivers/spi/spi_loopback/src/spi.c
@@ -142,6 +142,60 @@ static int spi_complete_loop(struct device *dev, struct spi_config *spi_conf)
 	return 0;
 }
 
+
+static int spi_null_tx_buf(struct device *dev, struct spi_config *spi_conf)
+{
+        static const u8_t EXPECTED_NOP_RETURN_BUF[BUF_SIZE] = { 0 };
+        (void)memset(buffer_rx, 0x77, BUF_SIZE);
+
+	const struct spi_buf tx_bufs[] = {
+		{
+                       /* According to documentation, when sending NULL tx buf -
+                          NOP frames should be sent on MOSI line */
+                        .buf = NULL,
+			.len = BUF_SIZE,
+		},
+	};
+	const struct spi_buf rx_bufs[] = {
+		{
+			.buf = buffer_rx,
+			.len = BUF_SIZE,
+		},
+	};
+	const struct spi_buf_set tx = {
+		.buffers = tx_bufs,
+		.count = ARRAY_SIZE(tx_bufs)
+	};
+	const struct spi_buf_set rx = {
+		.buffers = rx_bufs,
+		.count = ARRAY_SIZE(rx_bufs)
+	};
+
+	int ret;
+
+	LOG_INF("Start");
+
+	ret = spi_transceive(dev, spi_conf, &tx, &rx);
+	if (ret) {
+		LOG_ERR("Code %d", ret);
+		zassert_false(ret, "SPI transceive failed");
+		return ret;
+	}
+
+
+        if (memcmp(buffer_rx, EXPECTED_NOP_RETURN_BUF, BUF_SIZE))
+        {
+		to_display_format(buffer_rx, BUF_SIZE, buffer_print_rx);
+		LOG_ERR("Rx Buffer should contain NOP frames but got: %s", buffer_print_rx);
+		zassert_false(1, "Buffer not as expected");
+		return -1;
+        }
+
+	LOG_INF("Passed");
+
+	return 0;
+}
+
 static int spi_rx_half_start(struct device *dev, struct spi_config *spi_conf)
 {
 	const struct spi_buf tx_bufs[] = {
@@ -462,6 +516,7 @@ void test_spi_loopback(void)
 #endif
 
 	if (spi_complete_loop(spi_slow, &spi_cfg_slow) ||
+            spi_null_tx_buf(spi_slow, &spi_cfg_slow) ||
 	    spi_rx_half_start(spi_slow, &spi_cfg_slow) ||
 	    spi_rx_half_end(spi_slow, &spi_cfg_slow) ||
 	    spi_rx_every_4(spi_slow, &spi_cfg_slow)
@@ -473,6 +528,7 @@ void test_spi_loopback(void)
 	}
 
 	if (spi_complete_loop(spi_fast, &spi_cfg_fast) ||
+            spi_null_tx_buf(spi_fast, &spi_cfg_fast) ||
 	    spi_rx_half_start(spi_fast, &spi_cfg_fast) ||
 	    spi_rx_half_end(spi_fast, &spi_cfg_fast) ||
 	    spi_rx_every_4(spi_fast, &spi_cfg_fast)

Steps to reproduce the behavior

1. Connect MISO line to MOSI line on the board (loopback)
2. cd tests/drivers/spi/spi_loopback
3. west build -b nucleo_f413zh
4. west flash
5. see the test fails

Expected behavior
Test should pass: RX buffer should contain all zeros.

Impact
This is a showstopper! null pointer de-reference can lead to serious issues.

Environment

• OS: Linux (Ubuntu 18.04)
• Toolchain: Zephyr SDK 0.10.3
• Commit SHA or Version used: master 75bd9b6

Additional context
Proposed fix patch (go back to using spi_context_tx_buf_on()):

diff --git a/zephyrproject/zephyr/drivers/spi/spi_ll_stm32.c b/zephyrproject/zephyr/drivers/spi/spi_ll_stm32.c
index 7df2ea9d..27898d9f 100644
--- a/zephyrproject/zephyr/drivers/spi/spi_ll_stm32.c
+++ b/zephyrproject/zephyr/drivers/spi/spi_ll_stm32.c
@@ -96,14 +96,14 @@ static void spi_stm32_shift_m(SPI_TypeDef *spi, struct spi_stm32_data *data)
 #endif
 
     if (SPI_WORD_SIZE_GET(data->ctx.config->operation) == 8) {
-		if (spi_context_tx_on(&data->ctx)) {
+               if (spi_context_tx_buf_on(&data->ctx)) {
                     tx_frame = UNALIGNED_GET((u8_t *)(data->ctx.tx_buf));
         }
         LL_SPI_TransmitData8(spi, tx_frame);
         /* The update is ignored if TX is off. */
         spi_context_update_tx(&data->ctx, 1, 1);
     } else {
-		if (spi_context_tx_on(&data->ctx)) {
+               if (spi_context_tx_buf_on(&data->ctx)) {
                     tx_frame = UNALIGNED_GET((u16_t *)(data->ctx.tx_buf));
         }
         LL_SPI_TransmitData16(spi, tx_frame);

[tbursztyka]

Good catch. Would you mind sending a PR directly, since you have the right fix already?

[edit] there is something missing though, tx_frame needs to be filled with a 0 if spi_context_tx_buf_on() is false.

[a8961713]

[edit] there is something missing though, tx_frame needs to be filled with a 0 if spi_context_tx_buf_on() is false.

No need. tx_frame is already initialized to zero at the beginning of this function.

[tbursztyka]

indeed, confused it with rx_frame.

[erwango]

@a8961713, good catch, this should have actually been reverted as part of #21722.
Can you submit a PR ?