Messages in this thread | | | Subject | Re: [PATCH 1/3] lib/test_printf: Clean up test of hashed pointers | From | Rasmus Villemoes <> | Date | Tue, 3 Mar 2020 12:14:51 +0100 |
| |
On 27/02/2020 14.01, Petr Mladek wrote: > The commit ad67b74d2469d9b82a ("printk: hash addresses printed with %p") > helps to prevent leaking kernel addresses. > > The testing of this functionality is a bit problematic because the output > depends on a random key that is generated during boot. Though, it is > still possible to check some aspects: > > + output string length > + hash differs from the original pointer value > + top half bits are zeroed on 64-bit systems > > This is currently done by a maze of functions: > > + It is hard to follow. > + Some code is duplicated, e.g. the check for initialized crng. > + The zeroed top half bits are tested only with one hardcoded PTR. > + plain() increments "failed_tests" but not "total_tests". > + The generic test_hashed() does not touch number of tests at all. > > Move all the checks into test_hashed() so that they are done for > any given pointer that should get hashed. Also handle test counters > and internal errors the same way as the existing test() function. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > lib/test_printf.c | 130 ++++++++++++++++++------------------------------------ > 1 file changed, 42 insertions(+), 88 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 2d9f520d2f27..6fa6fb606554 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -215,29 +215,6 @@ test_string(void) > #define PTR_VAL_NO_CRNG "(____ptrval____)" > #define ZEROS "00000000" /* hex 32 zero bits */ > > -static int __init > -plain_format(void) > -{ > - char buf[PLAIN_BUF_SIZE]; > - int nchars; > - > - nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR); > - > - if (nchars != PTR_WIDTH) > - return -1; > - > - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", > - PTR_VAL_NO_CRNG); > - return 0; > - } > - > - if (strncmp(buf, ZEROS, strlen(ZEROS)) != 0) > - return -1; > - > - return 0; > -} > - > #else > > #define PTR_WIDTH 8 > @@ -246,88 +223,65 @@ plain_format(void) > #define PTR_VAL_NO_CRNG "(ptrval)" > #define ZEROS "" > > -static int __init > -plain_format(void) > -{ > - /* Format is implicitly tested for 32 bit machines by plain_hash() */ > - return 0; > -} > - > #endif /* BITS_PER_LONG == 64 */ > > -static int __init > -plain_hash_to_buffer(const void *p, char *buf, size_t len) > +static void __init > +test_hashed(const char *fmt, const void *p) > { > + char real[PLAIN_BUF_SIZE]; > + char hash[PLAIN_BUF_SIZE]; > int nchars; > > - nchars = snprintf(buf, len, "%p", p); > - > - if (nchars != PTR_WIDTH) > - return -1; > + total_tests++; > > - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", > - PTR_VAL_NO_CRNG); > - return 0; > + nchars = snprintf(real, sizeof(real), "%px", p); > + if (nchars != PTR_WIDTH) { > + pr_err("error in test suite: vsprintf(\"%%px\", p) returned number of characters %d, expected %d\n", > + nchars, PTR_WIDTH); > + goto err; > } > > - return 0; > -} > - > -static int __init > -plain_hash(void) > -{ > - char buf[PLAIN_BUF_SIZE]; > - int ret; > - > - ret = plain_hash_to_buffer(PTR, buf, PLAIN_BUF_SIZE); > - if (ret) > - return ret; > - > - if (strncmp(buf, PTR_STR, PTR_WIDTH) == 0) > - return -1; > - > - return 0; > -} > - > -/* > - * We can't use test() to test %p because we don't know what output to expect > - * after an address is hashed. > - */ > -static void __init > -plain(void) > -{ > - int err; > + nchars = snprintf(hash, sizeof(hash), fmt, p);
I don't like introducing a use of snprintf in the test suite where the compiler cannot do the basic type checking. In fact, I think we should turn on -Werror=format (or whatever the spelling is) for test_printf.c.
So I'd much rather introduce a
int check_hashed(const char *hashed, int ret, void *p)
helper and have the caller do the "%p", p formatting to a local buffer, pass that buffer and the snprintf return value along with the formatted pointer p to check_hashed, then do
failed_tests += check_hashed(...)
in the caller. Then you can use a "return 1" in the places where you now have a "goto err".
And I think you need a rather early check in check_hashed that there's a nul byte in the buffer that is being checked (as well as in the buffer containing the "%px" output) before you use those buffers as %s arguments in the error messages. do_test() carefully postpones the comparison to the expected content (and writing of the "expected ..., got ...") until after we at least know %s won't end up reading beyond the end of the buffer.
> + if (nchars != PTR_WIDTH) { > + pr_warn("vsprintf(\"%s\", p) returned number of characters %d, expected %d\n", > + fmt, nchars, PTR_WIDTH);
No, you did not call vsprintf. You called snprintf() - and vsprintf isn't even in the call chain of that. Given that there are functions in vsprintf.c that munge the return value (the s_c_nprintf family), please be more precise.
> + goto err; > + } > > - err = plain_hash(); > - if (err) { > - pr_warn("plain 'p' does not appear to be hashed\n"); > - failed_tests++; > + if (strncmp(hash, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > + pr_warn_once("crng possibly not yet initialized. vsprinf(\"%s\", p) printed \"%s\"", > + fmt, hash); > + total_tests--; > return; > }
Rather than decrementing total_tests, we should have a skipped_tests to account for the rare case(s) where we had to skip a test for some reason. Doing pr_warn_once for each such case is fine.
Also, typo (vsprinf), but use the right name anyway.
> > - err = plain_format(); > - if (err) { > - pr_warn("hashing plain 'p' has unexpected format\n"); > - failed_tests++; > + /* > + * There is a small chance of a false negative on 32-bit systems > + * when the hash is the same as the pointer value. > + */ > + if (strncmp(hash, real, PTR_WIDTH) == 0) { > + pr_warn("vsprintf(\"%s\", p) returned %s, expected hashed pointer\n", > + fmt, hash); > + goto err; > + } > + > +#if BITS_PER_LONG == 64 > + if (strncmp(hash, ZEROS, PTR_WIDTH / 2) != 0) { > + pr_warn("vsprintf(\"%s\", p) returned %s, expected %s in the top half bits\n", > + fmt, hash, ZEROS); > + goto err; > } > +#endif
OK, but should we also have a strspn(, "0123456789abcdef") check that the formatted string consists of precisely PTR_WIDTH hex decimals?
Rasmus
| |