| Message ID | 20210210051814.845713-1-timur@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | add support for never printing hashed addresses | expand |
On Tue, Feb 09, 2021 at 11:18PM -0600, Timur Tabi wrote: > [accidentally sent from the wrong email address, so resending] > > [The list of email addresses on CC: is getting quite lengthy, > so I hope I've included everyone.] > > Although hashing addresses printed via printk does make the > kernel more secure, it interferes with debugging, especially > with some functions like print_hex_dump() which always uses > hashed addresses. > > To avoid having to choose between %p and %px, it's easier to > add a kernel command line that treats all %p as %px. This > encourages developers to use %p more without making debugging > more difficult. > > Patches #1 and #2 upgrade the kselftest framework so that > it can report on tests that were skipped outright. This > is needed for the test_printf module which will now skip > %p hashing tests if hashing is disabled. > > Patch #2 upgrades the printf library to check the command > line. It also updates test_printf(). > > Timur Tabi (3): > lib/test_printf: use KSTM_MODULE_GLOBALS macro > kselftest: add support for skipped tests > [v2] lib/vsprintf: make-printk-non-secret printks all addresses as > unhashed > > .../admin-guide/kernel-parameters.txt | 15 +++++++ > lib/test_printf.c | 12 +++++- > lib/vsprintf.c | 40 ++++++++++++++++++- > tools/testing/selftests/kselftest_module.h | 18 ++++++--- > 4 files changed, 75 insertions(+), 10 deletions(-) I wanted to test this for deciding if we can show sensitive info in KFENCE reports, which works just fine now that debug_never_hash_pointers is non-static. FWIW, Acked-by: Marco Elver <elver@google.com> But unfortunately this series broke some other test: | In file included from lib/test_bitmap.c:17: | lib/test_bitmap.c: In function ‘test_bitmap_init’: | lib/../tools/testing/selftests/kselftest_module.h:45:48: error: ‘skipped_tests’ undeclared (first use in this function); did you mean ‘failed_tests’? | 45 | return kstm_report(total_tests, failed_tests, skipped_tests); \ | | ^~~~~~~~~~~~~ | lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’ | 637 | KSTM_MODULE_LOADERS(test_bitmap); | | ^~~~~~~~~~~~~~~~~~~ | lib/../tools/testing/selftests/kselftest_module.h:45:48: note: each undeclared identifier is reported only once for each function it appears in | 45 | return kstm_report(total_tests, failed_tests, skipped_tests); \ | | ^~~~~~~~~~~~~ | lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’ | 637 | KSTM_MODULE_LOADERS(test_bitmap); | | ^~~~~~~~~~~~~~~~~~~ | lib/../tools/testing/selftests/kselftest_module.h:46:1: error: control reaches end of non-void function [-Werror=return-type] | 46 | } \ | | ^ | lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’ | 637 | KSTM_MODULE_LOADERS(test_bitmap); | | ^~~~~~~~~~~~~~~~~~~ My allyesconfig build suggests test_bitmap.c is the only one, so it should probably be fixed up in this series. Thanks, -- Marco
On Wed, Feb 10, 2021 at 10:33 AM Timur Tabi <timur@kernel.org> wrote: > > [accidentally sent from the wrong email address, so resending] > > [The list of email addresses on CC: is getting quite lengthy, > so I hope I've included everyone.] > > Although hashing addresses printed via printk does make the > kernel more secure, it interferes with debugging, especially > with some functions like print_hex_dump() which always uses > hashed addresses. > > To avoid having to choose between %p and %px, it's easier to > add a kernel command line that treats all %p as %px. This > encourages developers to use %p more without making debugging > more difficult. > > Patches #1 and #2 upgrade the kselftest framework so that > it can report on tests that were skipped outright. This > is needed for the test_printf module which will now skip > %p hashing tests if hashing is disabled. > > Patch #2 upgrades the printf library to check the command > line. It also updates test_printf(). It's a bit hard in some mailers (like Gmail) to see the different versions of your patches. Can you use in the future - either `git format-patch -v<N> ...`, where <N> is a version - or `git format-patch --subject-prefix="PATCH vX / RESEND / etc" ...` ?
On 2021/02/10 14:18, Timur Tabi wrote: > [accidentally sent from the wrong email address, so resending] > > [The list of email addresses on CC: is getting quite lengthy, > so I hope I've included everyone.] > > Although hashing addresses printed via printk does make the > kernel more secure, it interferes with debugging, especially > with some functions like print_hex_dump() which always uses > hashed addresses. Oh, I was wishing diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3b53c73580c5..34c7e145ac3c 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -802,7 +802,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr, * Print the real pointer value for NULL and error pointers, * as they are not actual addresses. */ - if (IS_ERR_OR_NULL(ptr)) + if (IS_ERR_OR_NULL(ptr) || IS_ENABLED(CONFIG_DEBUG_DONT_HASH_POINTERS)) return pointer_string(buf, end, ptr, spec); /* When debugging early boot use non-cryptographically secure hash. */ change as a kernel config option, for more we try to switch using kernel command line options, more we likely make errors with sharing appropriate kernel command line options (e.g. https://github.com/google/syzkaller/commit/99c64d5c672700d6c0de63d11db25a0678e47a75 ).
On Thu, 11 Feb 2021 00:46:15 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Oh, I was wishing > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3b53c73580c5..34c7e145ac3c 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -802,7 +802,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr, > * Print the real pointer value for NULL and error pointers, > * as they are not actual addresses. > */ > - if (IS_ERR_OR_NULL(ptr)) > + if (IS_ERR_OR_NULL(ptr) || IS_ENABLED(CONFIG_DEBUG_DONT_HASH_POINTERS)) > return pointer_string(buf, end, ptr, spec); > > /* When debugging early boot use non-cryptographically secure hash. */ > > change as a kernel config option, for more we try to switch using kernel command line options, > more we likely make errors with sharing appropriate kernel command line options > (e.g. https://github.com/google/syzkaller/commit/99c64d5c672700d6c0de63d11db25a0678e47a75 ). The entire point of this exercise is not to make it easy to add this feature. Linus was absolutely against a config option, and I am too. The point of this exercise is to be able to debug the *same* kernel that someone is having issues with. And this is to facilitate that debugging. Whereas the example you show, the command line modifies how the kernel works. This command line only modifies what the kernel displays. Big difference. -- Steve
On 2021/02/11 1:18, Steven Rostedt wrote: > The point of this exercise is to be able to debug the *same* kernel that > someone is having issues with. And this is to facilitate that debugging. That's too difficult to use. If a problem is not reproducible, we will have no choice but always specify "never hash pointers" command line option. If a problem is reproducible, we can rebuild that kernel with "never hash pointers" config option turned on.
On Thu, 11 Feb 2021 01:39:41 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/02/11 1:18, Steven Rostedt wrote: > > The point of this exercise is to be able to debug the *same* kernel that > > someone is having issues with. And this is to facilitate that debugging. > > That's too difficult to use. If a problem is not reproducible, we will have > no choice but always specify "never hash pointers" command line option. If a > problem is reproducible, we can rebuild that kernel with "never hash pointers" > config option turned on. Now the question is, why do you need the unhashed pointer? Currently, the instruction pointer is what is fine right? You get the a function and its offset. If there's something that is needed, perhaps we should look at how to fix that, instead of just unhashing all pointers by default. -- Steve
On Thu, Feb 11, 2021 at 01:39:41AM +0900, Tetsuo Handa wrote: > On 2021/02/11 1:18, Steven Rostedt wrote: > > The point of this exercise is to be able to debug the *same* kernel that > > someone is having issues with. And this is to facilitate that debugging. > > That's too difficult to use. If a problem is not reproducible, we will have > no choice but always specify "never hash pointers" command line option. If a > problem is reproducible, we can rebuild that kernel with "never hash pointers" > config option turned on. I think what you are targeting is something like dynamic debug approach where you can choose which prints to enable/disable and what enable/disable in them. In that case you specifically apply a command line option and enable only files / lines in the files.
On 2/10/21 5:47 AM, Andy Shevchenko wrote: > It's a bit hard in some mailers (like Gmail) to see the different > versions of your patches. > Can you use in the future > - either `git format-patch -v<N> ...`, where <N> is a version > - or `git format-patch --subject-prefix="PATCH vX / RESEND / etc" ...` > ? Yes, of course. Sorry about that. Like I said earlier, I really shouldn't be posting patches late at night, when I will just forget important details.
On 2021/02/11 1:46, Steven Rostedt wrote: > On Thu, 11 Feb 2021 01:39:41 +0900 > Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> On 2021/02/11 1:18, Steven Rostedt wrote: >>> The point of this exercise is to be able to debug the *same* kernel that >>> someone is having issues with. And this is to facilitate that debugging. >> >> That's too difficult to use. If a problem is not reproducible, we will have >> no choice but always specify "never hash pointers" command line option. If a >> problem is reproducible, we can rebuild that kernel with "never hash pointers" >> config option turned on. > > Now the question is, why do you need the unhashed pointer? Because unhashed pointers might give some clue. We can rebuild the same kernel using the same kernel config / compiler etc. and compare unhashed pointers with addresses in System.map / kallsyms files without reproducing the problem. > > Currently, the instruction pointer is what is fine right? You get the > a function and its offset. If there's something that is needed, perhaps we > should look at how to fix that, instead of just unhashing all pointers by > default. I'm not refusing to use kernel command line options. I'm expecting that we can also hardcode using kernel config options. Since boot-time switching via kernel command line options makes the kernel behave differently, less boot-time switching is better for avoiding unexpected problems (e.g. unintended LSM was enabled).
On 2/10/21 10:46 AM, Steven Rostedt wrote: > Now the question is, why do you need the unhashed pointer? > > Currently, the instruction pointer is what is fine right? You get the > a function and its offset. If there's something that is needed, perhaps we > should look at how to fix that, instead of just unhashing all pointers by > default. The original version of this patch only fixed print_hex_dump(), because hashed addresses didn't make any sense for that. Each address is incremented by 16 or 32, but since they were all hashed, they may as well have been random numbers.
On Thu, 11 Feb 2021 02:07:21 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > I'm not refusing to use kernel command line options. I'm expecting that we can > also hardcode using kernel config options. Since boot-time switching via kernel > command line options makes the kernel behave differently, less boot-time > switching is better for avoiding unexpected problems (e.g. unintended LSM was > enabled). You could just add: CONFIG_CMDLINE_BOOL=y CONFIG_CMDLINE="make-printk-non-secret" If you want it part of your config. -- Steve
On 2021/02/11 1:54, Andy Shevchenko wrote: > On Thu, Feb 11, 2021 at 01:39:41AM +0900, Tetsuo Handa wrote: >> On 2021/02/11 1:18, Steven Rostedt wrote: >>> The point of this exercise is to be able to debug the *same* kernel that >>> someone is having issues with. And this is to facilitate that debugging. >> >> That's too difficult to use. If a problem is not reproducible, we will have >> no choice but always specify "never hash pointers" command line option. If a >> problem is reproducible, we can rebuild that kernel with "never hash pointers" >> config option turned on. > > I think what you are targeting is something like dynamic debug approach where > you can choose which prints to enable/disable and what enable/disable in them. What I'm targeting is "zero interaction from kernel command line options" like https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/usbip?id=f1bdf414e7dd0cbc26460425719fc3ea479947a2 . > > In that case you specifically apply a command line option and enable only files > / lines in the files. While there is boot-config feature for specifying very long kernel command line options, I can't enforce syzkaller users (including syzbot) to switch what to enable/disable via kernel command line options. Let alone defining a kernel command line option for single-purpose debug printk() changes like shown above.
On 2/10/21 5:11 AM, Marco Elver wrote: > I wanted to test this for deciding if we can show sensitive info in > KFENCE reports, which works just fine now that debug_never_hash_pointers > is non-static. FWIW, > > Acked-by: Marco Elver<elver@google.com> Thank you. > But unfortunately this series broke some other test: > > | In file included from lib/test_bitmap.c:17: > | lib/test_bitmap.c: In function ‘test_bitmap_init’: > | lib/../tools/testing/selftests/kselftest_module.h:45:48: error: ‘skipped_tests’ undeclared (first use in this function); did you mean ‘failed_tests’? This will be fixed in v3. Thanks.
[accidentally sent from the wrong email address, so resending] [The list of email addresses on CC: is getting quite lengthy, so I hope I've included everyone.] Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. To avoid having to choose between %p and %px, it's easier to add a kernel command line that treats all %p as %px. This encourages developers to use %p more without making debugging more difficult. Patches #1 and #2 upgrade the kselftest framework so that it can report on tests that were skipped outright. This is needed for the test_printf module which will now skip %p hashing tests if hashing is disabled. Patch #2 upgrades the printf library to check the command line. It also updates test_printf(). Timur Tabi (3): lib/test_printf: use KSTM_MODULE_GLOBALS macro kselftest: add support for skipped tests [v2] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed .../admin-guide/kernel-parameters.txt | 15 +++++++ lib/test_printf.c | 12 +++++- lib/vsprintf.c | 40 ++++++++++++++++++- tools/testing/selftests/kselftest_module.h | 18 ++++++--- 4 files changed, 75 insertions(+), 10 deletions(-)