| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Feb 2024 14:07:38 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Mark Brown <broonie@...nel.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] regmap: kunit: Ensure that changed bytes are actually
different
Hi Mark,
On 2/9/24 13:33, Mark Brown wrote:
> During the cache sync test we verify that values we expect to have been
> written only to the cache do not appear in the hardware. This works most
> of the time but since we randomly generate both the original and new values
> there is a low probability that these values may actually be the same.
> Wrap get_random_bytes() to ensure that the values are different, it is
> likely we will want a similar pattern for other tests in the future.
>
> We use random generation to try to avoid data dependencies in the tests.
>
> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Mark Brown <broonie@...nel.org>
This is actually worse than v1 because hw_buf[6] isn't used anywhere.
Making sure that the values in the val[] array don't match the values
in hw_buf[6..7] doesn't add any value.
With this change, and the patch below applied on top of it:
# raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1329
Expected &hw_buf[2] != val, but
&hw_buf[2] ==
fb 16 cf 93
val ==
fb 16 cf 93
# raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1330
Expected &hw_buf[4] != val, but
&hw_buf[4] ==
fb 16
val ==
fb 16
not ok 1 flat-little
# raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1329
[ lots of those ]
FWIW, I had struggled with the re-use of val[0] for two different tests
(on hw_buf[2] and hw_buf[4]) myself. The only solution other than making sure
that it neither matches hw_buf[2] nor hw_buf[4] I came up with was to use a
separate variable for the accesses to hw_buf[4] (or hw_buf[6] in the old code).
Something like the second patch below, applied on top of your v2.
Guenter
---
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 85e02f86b14c..67cc3239f478 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -1284,6 +1284,14 @@ static void raw_sync(struct kunit *test)
hw_buf = (u16 *)data->vals;
get_changed_bytes(&hw_buf[6], &val[0], sizeof(val));
+ // Let's cheat.
+ // Remember, the above code doesn't look into hw_buf[2..5],
+ // so anything might be in there, including the values from
+ // the val[] array.
+ hw_buf[2] = val[0];
+ hw_buf[3] = val[1];
+ hw_buf[4] = val[0];
+ hw_buf[5] = val[1];
/* Do a regular write and a raw write in cache only mode */
regcache_cache_only(map, true);
---
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 85e02f86b14c..d2cf510f86f0 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -1269,7 +1269,7 @@ static void raw_sync(struct kunit *test)
struct regmap *map;
struct regmap_config config;
struct regmap_ram_data *data;
- u16 val[2];
+ u16 val[3];
u16 *hw_buf;
unsigned int rval;
int i;
@@ -1283,17 +1283,17 @@ static void raw_sync(struct kunit *test)
hw_buf = (u16 *)data->vals;
- get_changed_bytes(&hw_buf[6], &val[0], sizeof(val));
+ get_changed_bytes(&hw_buf[2], &val[0], sizeof(val));
/* Do a regular write and a raw write in cache only mode */
regcache_cache_only(map, true);
- KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(val)));
+ KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(u16) * 2));
if (config.val_format_endian == REGMAP_ENDIAN_BIG)
KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
- be16_to_cpu(val[0])));
+ be16_to_cpu(val[2])));
else
KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
- le16_to_cpu(val[0])));
+ le16_to_cpu(val[2])));
/* We should read back the new values, and defaults for the rest */
for (i = 0; i < config.max_register + 1; i++) {
@@ -1305,10 +1305,10 @@ static void raw_sync(struct kunit *test)
case 4:
if (config.val_format_endian == REGMAP_ENDIAN_BIG) {
KUNIT_EXPECT_EQ(test, rval,
- be16_to_cpu(val[i % 2]));
+ be16_to_cpu(val[i - 2]));
} else {
KUNIT_EXPECT_EQ(test, rval,
- le16_to_cpu(val[i % 2]));
+ le16_to_cpu(val[i - 2]));
}
break;
default:
@@ -1318,8 +1318,8 @@ static void raw_sync(struct kunit *test)
}
/* The values should not appear in the "hardware" */
- KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], val, sizeof(val));
- KUNIT_EXPECT_MEMNEQ(test, &hw_buf[4], val, sizeof(u16));
+ KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], val, sizeof(u16) * 2);
+ KUNIT_EXPECT_MEMNEQ(test, &hw_buf[4], &val[2], sizeof(u16));
for (i = 0; i < config.max_register + 1; i++)
data->written[i] = false;
@@ -1331,7 +1331,6 @@ static void raw_sync(struct kunit *test)
/* The values should now appear in the "hardware" */
KUNIT_EXPECT_MEMEQ(test, &hw_buf[2], val, sizeof(val));
- KUNIT_EXPECT_MEMEQ(test, &hw_buf[4], val, sizeof(u16));
regmap_exit(map);
}
Powered by blists - more mailing lists