| Message ID | 20191121235058.21653-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2d68df6cc4bf5822d78cd0f067174d6e29a2f739 |
| Headers | show |
| Series | [kselftest/test] kunit: Always print actual pointer values in asserts | expand |
On Thu, Nov 21, 2019 at 3:51 PM David Gow <davidgow@google.com> wrote: > > KUnit assertions and expectations will print the values being tested. If > these are pointers (e.g., KUNIT_EXPECT_PTR_EQ(test, a, b)), these > pointers are currently printed with the %pK format specifier, which -- to > prevent information leaks which may compromise, e.g., ASLR -- are often > either hashed or replaced with ____ptrval____ or similar, making debugging > tests difficult. > > By replacing %pK with %px as Documentation/core-api/printk-formats.rst > suggests, we disable this security feature for KUnit assertions and > expectations, allowing the actual pointer values to be printed. Given > that KUnit is not intended for use in production kernels, and the > pointers are only printed on failing tests, this seems like a worthwhile > tradeoff. I agree. However, I also remember that others in the past yelled at me for assuming that KUnit would not be built into production kernels. I feel like +Kees Cook would have a good opinion on this (or will at least CC the right people). > > Signed-off-by: David Gow <davidgow@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> Thanks!
On Tue, Dec 3, 2019 at 3:44 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Thu, Nov 21, 2019 at 3:51 PM David Gow <davidgow@google.com> wrote: > > > > KUnit assertions and expectations will print the values being tested. If > > these are pointers (e.g., KUNIT_EXPECT_PTR_EQ(test, a, b)), these > > pointers are currently printed with the %pK format specifier, which -- to > > prevent information leaks which may compromise, e.g., ASLR -- are often > > either hashed or replaced with ____ptrval____ or similar, making debugging > > tests difficult. > > > > By replacing %pK with %px as Documentation/core-api/printk-formats.rst > > suggests, we disable this security feature for KUnit assertions and > > expectations, allowing the actual pointer values to be printed. Given > > that KUnit is not intended for use in production kernels, and the > > pointers are only printed on failing tests, this seems like a worthwhile > > tradeoff. > > I agree. However, I also remember that others in the past yelled at me > for assuming that KUnit would not be built into production kernels. > > I feel like +Kees Cook would have a good opinion on this (or will at > least CC the right people). > I'm tempted to take the silence as a sign that no-one is upset by this. Otherwise, consider this a gentle reminder to file any objections you may have. :-) Otherwise, I've confirmed that this still applies cleanly to the latest linux-kselftest/kunit branch, so -- assuming there are no last-minute objections -- this ought to be ready to go. Cheers, -- David
On Tue, Mar 24, 2020 at 8:33 PM David Gow <davidgow@google.com> wrote: > > On Tue, Dec 3, 2019 at 3:44 PM Brendan Higgins > <brendanhiggins@google.com> wrote: > > > > On Thu, Nov 21, 2019 at 3:51 PM David Gow <davidgow@google.com> wrote: > > > > > > KUnit assertions and expectations will print the values being tested. If > > > these are pointers (e.g., KUNIT_EXPECT_PTR_EQ(test, a, b)), these > > > pointers are currently printed with the %pK format specifier, which -- to > > > prevent information leaks which may compromise, e.g., ASLR -- are often > > > either hashed or replaced with ____ptrval____ or similar, making debugging > > > tests difficult. > > > > > > By replacing %pK with %px as Documentation/core-api/printk-formats.rst > > > suggests, we disable this security feature for KUnit assertions and > > > expectations, allowing the actual pointer values to be printed. Given > > > that KUnit is not intended for use in production kernels, and the > > > pointers are only printed on failing tests, this seems like a worthwhile > > > tradeoff. > > > > I agree. However, I also remember that others in the past yelled at me > > for assuming that KUnit would not be built into production kernels. > > > > I feel like +Kees Cook would have a good opinion on this (or will at > > least CC the right people). > > > > I'm tempted to take the silence as a sign that no-one is upset by > this. Otherwise, consider this a gentle reminder to file any > objections you may have. :-) > > Otherwise, I've confirmed that this still applies cleanly to the > latest linux-kselftest/kunit branch, so -- assuming there are no > last-minute objections -- this ought to be ready to go. Shuah, can you pick this up for 5.7?
On 3/25/20 10:33 AM, Brendan Higgins wrote: > On Tue, Mar 24, 2020 at 8:33 PM David Gow <davidgow@google.com> wrote: >> >> On Tue, Dec 3, 2019 at 3:44 PM Brendan Higgins >> <brendanhiggins@google.com> wrote: >>> >>> On Thu, Nov 21, 2019 at 3:51 PM David Gow <davidgow@google.com> wrote: >>>> >>>> KUnit assertions and expectations will print the values being tested. If >>>> these are pointers (e.g., KUNIT_EXPECT_PTR_EQ(test, a, b)), these >>>> pointers are currently printed with the %pK format specifier, which -- to >>>> prevent information leaks which may compromise, e.g., ASLR -- are often >>>> either hashed or replaced with ____ptrval____ or similar, making debugging >>>> tests difficult. >>>> >>>> By replacing %pK with %px as Documentation/core-api/printk-formats.rst >>>> suggests, we disable this security feature for KUnit assertions and >>>> expectations, allowing the actual pointer values to be printed. Given >>>> that KUnit is not intended for use in production kernels, and the >>>> pointers are only printed on failing tests, this seems like a worthwhile >>>> tradeoff. >>> >>> I agree. However, I also remember that others in the past yelled at me >>> for assuming that KUnit would not be built into production kernels. >>> >>> I feel like +Kees Cook would have a good opinion on this (or will at >>> least CC the right people). >>> >> >> I'm tempted to take the silence as a sign that no-one is upset by >> this. Otherwise, consider this a gentle reminder to file any >> objections you may have. :-) >> >> Otherwise, I've confirmed that this still applies cleanly to the >> latest linux-kselftest/kunit branch, so -- assuming there are no >> last-minute objections -- this ought to be ready to go. > > Shuah, can you pick this up for 5.7? > Yes. I will pick this up. thanks, -- Shuah
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 86013d4cf891..a87960409bd4 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -110,10 +110,10 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, binary_assert->left_text, binary_assert->operation, binary_assert->right_text); - string_stream_add(stream, "\t\t%s == %pK\n", + string_stream_add(stream, "\t\t%s == %px\n", binary_assert->left_text, binary_assert->left_value); - string_stream_add(stream, "\t\t%s == %pK", + string_stream_add(stream, "\t\t%s == %px", binary_assert->right_text, binary_assert->right_value); kunit_assert_print_msg(assert, stream);
KUnit assertions and expectations will print the values being tested. If these are pointers (e.g., KUNIT_EXPECT_PTR_EQ(test, a, b)), these pointers are currently printed with the %pK format specifier, which -- to prevent information leaks which may compromise, e.g., ASLR -- are often either hashed or replaced with ____ptrval____ or similar, making debugging tests difficult. By replacing %pK with %px as Documentation/core-api/printk-formats.rst suggests, we disable this security feature for KUnit assertions and expectations, allowing the actual pointer values to be printed. Given that KUnit is not intended for use in production kernels, and the pointers are only printed on failing tests, this seems like a worthwhile tradeoff. Signed-off-by: David Gow <davidgow@google.com> --- This seems like the best way of solving this problem to me, but if anyone has a better solution I'd love to hear it. Note also that this does trigger two checkpatch.pl warnings, which warn that the change will potentially cause the kernel memory layout to be exposed. Since that's the whole point of the change, they probably sohuld stay there. lib/kunit/assert.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)