[PATCH net 0/2] net: mctp-serial: Fix for missing tx escapes

[PATCH net 0/2] net: mctp-serial: Fix for missing tx escapes

[Matt Johnston]

The mctp-serial code to add escape characters was incorrect due to an
off-by-one error. This series adds a test for the chunking which splits
by escape characters, and fixes the bug.

Matt Johnston (2):
  net: mctp-serial: Add kunit test for next_chunk_len()
  net: mctp-serial: Fix missing escapes on transmit

 drivers/net/mctp/Kconfig       |   5 ++
 drivers/net/mctp/mctp-serial.c | 113 ++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

[PATCH net 1/2] net: mctp-serial: Add kunit test for next_chunk_len()

[Matt Johnston]

Test various edge cases of inputs that contain characters
that need escaping.

This adds a new kunit suite for mctp-serial.

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 drivers/net/mctp/Kconfig       |   5 ++
 drivers/net/mctp/mctp-serial.c | 109 +++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index ce9d2d2ccf3b..15860d6ac39f 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -21,6 +21,11 @@ config MCTP_SERIAL
 	  Say y here if you need to connect to MCTP endpoints over serial. To
 	  compile as a module, use m; the module will be called mctp-serial.
 
+config MCTP_SERIAL_TEST
+        bool "MCTP serial tests" if !KUNIT_ALL_TESTS
+        depends on MCTP_SERIAL=y && KUNIT=y
+        default KUNIT_ALL_TESTS
+
 config MCTP_TRANSPORT_I2C
 	tristate "MCTP SMBus/I2C transport"
 	# i2c-mux is optional, but we must build as a module if i2c-mux is a module
diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
index 5bf6fdff701c..d7db11355909 100644
--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c
@@ -521,3 +521,112 @@ module_exit(mctp_serial_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Jeremy Kerr <jk@codeconstruct.com.au>");
 MODULE_DESCRIPTION("MCTP Serial transport");
+
+#if IS_ENABLED(CONFIG_MCTP_SERIAL_TEST)
+#include <kunit/test.h>
+
+#define MAX_CHUNKS 6
+struct test_chunk_tx {
+	u8 input_len;
+	u8 input[MCTP_SERIAL_MTU];
+	u8 chunks[MAX_CHUNKS];
+};
+
+static void test_next_chunk_len(struct kunit *test)
+{
+	struct mctp_serial devx;
+	struct mctp_serial *dev = &devx;
+	int next;
+
+	struct test_chunk_tx *params = test->param_value;
+
+	memset(dev, 0x0, sizeof(*dev));
+	memcpy(dev->txbuf, params->input, params->input_len);
+	dev->txlen = params->input_len;
+
+	for (size_t i = 0; i < MAX_CHUNKS; i++) {
+		next = next_chunk_len(dev);
+		dev->txpos += next;
+		KUNIT_EXPECT_EQ(test, next, params->chunks[i]);
+
+		if (next == 0) {
+			KUNIT_EXPECT_EQ(test, dev->txpos, dev->txlen);
+			return;
+		}
+	}
+
+	KUNIT_FAIL_AND_ABORT(test, "Ran out of chunks");
+}
+
+static struct test_chunk_tx chunk_tx_tests[] = {
+	{
+		.input_len = 5,
+		.input = { 0x00, 0x11, 0x22, 0x7e, 0x80 },
+		.chunks = { 3, 1, 1, 0},
+	},
+	{
+		.input_len = 5,
+		.input = { 0x00, 0x11, 0x22, 0x7e, 0x7d },
+		.chunks = { 3, 1, 1, 0},
+	},
+	{
+		.input_len = 3,
+		.input = { 0x7e, 0x11, 0x22, },
+		.chunks = { 1, 2, 0},
+	},
+	{
+		.input_len = 3,
+		.input = { 0x7e, 0x7e, 0x7d, },
+		.chunks = { 1, 1, 1, 0},
+	},
+	{
+		.input_len = 4,
+		.input = { 0x7e, 0x7e, 0x00, 0x7d, },
+		.chunks = { 1, 1, 1, 1, 0},
+	},
+	{
+		.input_len = 6,
+		.input = { 0x7e, 0x7e, 0x00, 0x7d, 0x10, 0x10},
+		.chunks = { 1, 1, 1, 1, 2, 0},
+	},
+	{
+		.input_len = 1,
+		.input = { 0x7e },
+		.chunks = { 1, 0 },
+	},
+	{
+		.input_len = 1,
+		.input = { 0x80 },
+		.chunks = { 1, 0 },
+	},
+	{
+		.input_len = 3,
+		.input = { 0x80, 0x80, 0x00 },
+		.chunks = { 3, 0 },
+	},
+	{
+		.input_len = 7,
+		.input = { 0x01, 0x00, 0x08, 0xc8, 0x00, 0x80, 0x02 },
+		.chunks = { 7, 0 },
+	},
+	{
+		.input_len = 7,
+		.input = { 0x01, 0x00, 0x08, 0xc8, 0x7e, 0x80, 0x02 },
+		.chunks = { 4, 1, 2, 0 },
+	},
+};
+
+KUNIT_ARRAY_PARAM(chunk_tx, chunk_tx_tests, NULL);
+
+static struct kunit_case mctp_serial_test_cases[] = {
+	KUNIT_CASE_PARAM(test_next_chunk_len, chunk_tx_gen_params),
+};
+
+static struct kunit_suite mctp_serial_test_suite = {
+	.name = "mctp_serial",
+	.test_cases = mctp_serial_test_cases,
+};
+
+kunit_test_suite(mctp_serial_test_suite);
+
+#endif /* CONFIG_MCTP_SERIAL_TEST */

[PATCH net 2/2] net: mctp-serial: Fix missing escapes on transmit

[Matt Johnston]

0x7d and 0x7e bytes are meant to be escaped in the data portion of
frames, but this didn't occur since next_chunk_len() had an off-by-one
error. That also resulted in the final byte of a payload being written
as a separate tty write op.

The chunk prior to an escaped byte would be one byte short, and the
next call would never test the txpos+1 case, which is where the escaped
byte was located. That meant it never hit the escaping case in
mctp_serial_tx_work().

Example Input: 01 00 08 c8 7e 80 02

Previous incorrect chunks from next_chunk_len():

01 00 08
c8 7e 80
02

With this fix:

01 00 08 c8
7e
80 02

Cc: stable@vger.kernel.org
Fixes: a0c2ccd9b5ad ("mctp: Add MCTP-over-serial transport binding")
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-serial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
index d7db11355909..82890e983847 100644
--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c
@@ -91,8 +91,8 @@ static int next_chunk_len(struct mctp_serial *dev)
 	 * will be those non-escaped bytes, and does not include the escaped
 	 * byte.
 	 */
-	for (i = 1; i + dev->txpos + 1 < dev->txlen; i++) {
-		if (needs_escape(dev->txbuf[dev->txpos + i + 1]))
+	for (i = 1; i + dev->txpos < dev->txlen; i++) {
+		if (needs_escape(dev->txbuf[dev->txpos + i]))
 			break;
 	}
 

Re: [PATCH net 2/2] net: mctp-serial: Fix missing escapes on transmit

[Larysa Zaremba]

On Tue, Aug 27, 2024 at 10:07:59AM +0800, Matt Johnston wrote:
> 0x7d and 0x7e bytes are meant to be escaped in the data portion of
> frames, but this didn't occur since next_chunk_len() had an off-by-one
> error. That also resulted in the final byte of a payload being written
> as a separate tty write op.
> 
> The chunk prior to an escaped byte would be one byte short, and the
> next call would never test the txpos+1 case, which is where the escaped
> byte was located. That meant it never hit the escaping case in
> mctp_serial_tx_work().
> 
> Example Input: 01 00 08 c8 7e 80 02
> 
> Previous incorrect chunks from next_chunk_len():
> 
> 01 00 08
> c8 7e 80
> 02
> 
> With this fix:
> 
> 01 00 08 c8
> 7e
> 80 02
> 
> Cc: stable@vger.kernel.org
> Fixes: a0c2ccd9b5ad ("mctp: Add MCTP-over-serial transport binding")
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

> ---
>  drivers/net/mctp/mctp-serial.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index d7db11355909..82890e983847 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -91,8 +91,8 @@ static int next_chunk_len(struct mctp_serial *dev)
>  	 * will be those non-escaped bytes, and does not include the escaped
>  	 * byte.
>  	 */
> -	for (i = 1; i + dev->txpos + 1 < dev->txlen; i++) {
> -		if (needs_escape(dev->txbuf[dev->txpos + i + 1]))
> +	for (i = 1; i + dev->txpos < dev->txlen; i++) {
> +		if (needs_escape(dev->txbuf[dev->txpos + i]))
>  			break;
>  	}
>  
> 

Re: [PATCH net 1/2] net: mctp-serial: Add kunit test for next_chunk_len()

[kernel test robot]

Hi Matt,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Matt-Johnston/net-mctp-serial-Add-kunit-test-for-next_chunk_len/20240827-101656
base:   net/main
patch link:    https://lore.kernel.org/r/20240827020803.957250-2-matt%40codeconstruct.com.au
patch subject: [PATCH net 1/2] net: mctp-serial: Add kunit test for next_chunk_len()
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240828/202408280734.ioysUDjL-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280734.ioysUDjL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280734.ioysUDjL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/mctp/mctp-serial.c: In function 'test_next_chunk_len':
>> drivers/net/mctp/mctp-serial.c:541:40: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     541 |         struct test_chunk_tx *params = test->param_value;
         |                                        ^~~~


vim +/const +541 drivers/net/mctp/mctp-serial.c

   534	
   535	static void test_next_chunk_len(struct kunit *test)
   536	{
   537		struct mctp_serial devx;
   538		struct mctp_serial *dev = &devx;
   539		int next;
   540	
 > 541		struct test_chunk_tx *params = test->param_value;
   542	
   543		memset(dev, 0x0, sizeof(*dev));
   544		memcpy(dev->txbuf, params->input, params->input_len);
   545		dev->txlen = params->input_len;
   546	
   547		for (size_t i = 0; i < MAX_CHUNKS; i++) {
   548			next = next_chunk_len(dev);
   549			dev->txpos += next;
   550			KUNIT_EXPECT_EQ(test, next, params->chunks[i]);
   551	
   552			if (next == 0) {
   553				KUNIT_EXPECT_EQ(test, dev->txpos, dev->txlen);
   554				return;
   555			}
   556		}
   557	
   558		KUNIT_FAIL_AND_ABORT(test, "Ran out of chunks");
   559	}
   560	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net 1/2] net: mctp-serial: Add kunit test for next_chunk_len()

[kernel test robot]

Hi Matt,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Matt-Johnston/net-mctp-serial-Add-kunit-test-for-next_chunk_len/20240827-101656
base:   net/main
patch link:    https://lore.kernel.org/r/20240827020803.957250-2-matt%40codeconstruct.com.au
patch subject: [PATCH net 1/2] net: mctp-serial: Add kunit test for next_chunk_len()
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240829/202408291542.2o5GOBhQ-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240829/202408291542.2o5GOBhQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408291542.2o5GOBhQ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/mctp/mctp-serial.c:541:24: error: initializing 'struct test_chunk_tx *' with an expression of type 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
     541 |         struct test_chunk_tx *params = test->param_value;
         |                               ^        ~~~~~~~~~~~~~~~~~
   1 error generated.


vim +541 drivers/net/mctp/mctp-serial.c

   534	
   535	static void test_next_chunk_len(struct kunit *test)
   536	{
   537		struct mctp_serial devx;
   538		struct mctp_serial *dev = &devx;
   539		int next;
   540	
 > 541		struct test_chunk_tx *params = test->param_value;
   542	
   543		memset(dev, 0x0, sizeof(*dev));
   544		memcpy(dev->txbuf, params->input, params->input_len);
   545		dev->txlen = params->input_len;
   546	
   547		for (size_t i = 0; i < MAX_CHUNKS; i++) {
   548			next = next_chunk_len(dev);
   549			dev->txpos += next;
   550			KUNIT_EXPECT_EQ(test, next, params->chunks[i]);
   551	
   552			if (next == 0) {
   553				KUNIT_EXPECT_EQ(test, dev->txpos, dev->txlen);
   554				return;
   555			}
   556		}
   557	
   558		KUNIT_FAIL_AND_ABORT(test, "Ran out of chunks");
   559	}
   560	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki