| Message ID | 1509317956-28041-1-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Oct 29, 2017 at 3:59 PM, Tobin C. Harding <me@tobin.cc> wrote: > Currently there are many places in the kernel where addresses are being > printed using an unadorned %p. Kernel pointers should be printed using > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses > gives attackers sensitive information about the kernel layout in memory. > > We can reduce the attack surface by hashing all addresses printed with > %p. This will of course break some users, forcing code printing needed > addresses to be updated. > > For what it's worth, usage of unadorned %p can be broken down as > follows (thanks to Joe Perches). > > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c > 1084 arch > 20 block > 10 crypto > 32 Documentation > 8121 drivers > 1221 fs > 143 include > 101 kernel > 69 lib > 100 mm > 1510 net > 40 samples > 7 scripts > 11 security > 166 sound > 152 tools > 2 virt > > Add function ptr_to_id() to map an address to a 32 bit unique > identifier. Hash any unadorned usage of specifier %p and any malformed > specifiers. > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > --- > > It seems we don't have consensus on a couple of things > > 1. The size of the hashed address on 64 bit architectures. > 2. The use of '0x' pre-fix for hashed addresses. > > In regards to (1), we are agreed that we only need 32 bits of > information. There is some questions however that outputting _only_ 32 > bits may break userland. > > In regards to (2), irrespective of the arguments for and against, if > point 1 is correct and changing the format will break userland then we > can't add the '0x' suffix for the same reason. > > Therefore this patch masks off the first 32 bits, retaining > only 32 bits of information. We do not add a '0x' suffix. All in all, > that results in _no_ change to the format of output only the content of > the output. > > The leading 0's also make explicit that we have messed with the address, > maybe this will save some debugging time by doing so. Although this > would probably already be obvious since there is no leading 'ffff'. > > We hash malformed specifiers also. Malformed specifiers include > incomplete (e.g %pi) and also non-existent specifiers. checkpatch should > warn for non-existent specifiers but AFAICT won't warn for incomplete > specifiers. > > Here is the behaviour that this patch implements. > > For kpt_restrict==0 > > Randomness not ready: > printed with %p: (pointer value) # NOTE: with padding > Valid pointer: > printed with %pK: deadbeefdeadbeef > printed with %p: 00000000deadbeef > malformed specifier (eg %i): 00000000deadbeef > NULL pointer: > printed with %pK: 0000000000000000 > printed with %p: (null) # NOTE: with padding > malformed specifier (eg %i): (null) > > For kpt_restrict==2 > > Valid pointer: > printed with %pK: 0000000000000000 > > All other output as for kptr_restrict==0 > > V9: > - Drop the initial patch from V8, leaving null pointer handling as is. > - Print the hashed ID _without_ a '0x' suffix. > - Mask the first 32 bits of the hashed ID to all zeros on 64 bit > architectures. Oops, I had missed v9. This addresses my concerns. I think the leading zeros are a good way to identify the "this is clearly not a kernel address" issue (though the 32-bit folks may remain confused, but we can fix that later, IMO). -Kees
On Mon, Oct 30, 2017 at 03:31:41PM -0700, Kees Cook wrote: > On Sun, Oct 29, 2017 at 3:59 PM, Tobin C. Harding <me@tobin.cc> wrote: > > Currently there are many places in the kernel where addresses are being > > printed using an unadorned %p. Kernel pointers should be printed using > > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses > > gives attackers sensitive information about the kernel layout in memory. > > > > We can reduce the attack surface by hashing all addresses printed with > > %p. This will of course break some users, forcing code printing needed > > addresses to be updated. > > > > For what it's worth, usage of unadorned %p can be broken down as > > follows (thanks to Joe Perches). > > > > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c > > 1084 arch > > 20 block > > 10 crypto > > 32 Documentation > > 8121 drivers > > 1221 fs > > 143 include > > 101 kernel > > 69 lib > > 100 mm > > 1510 net > > 40 samples > > 7 scripts > > 11 security > > 166 sound > > 152 tools > > 2 virt > > > > Add function ptr_to_id() to map an address to a 32 bit unique > > identifier. Hash any unadorned usage of specifier %p and any malformed > > specifiers. > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > > > --- > > > > It seems we don't have consensus on a couple of things > > > > 1. The size of the hashed address on 64 bit architectures. > > 2. The use of '0x' pre-fix for hashed addresses. > > > > In regards to (1), we are agreed that we only need 32 bits of > > information. There is some questions however that outputting _only_ 32 > > bits may break userland. > > > > In regards to (2), irrespective of the arguments for and against, if > > point 1 is correct and changing the format will break userland then we > > can't add the '0x' suffix for the same reason. > > > > Therefore this patch masks off the first 32 bits, retaining > > only 32 bits of information. We do not add a '0x' suffix. All in all, > > that results in _no_ change to the format of output only the content of > > the output. > > > > The leading 0's also make explicit that we have messed with the address, > > maybe this will save some debugging time by doing so. Although this > > would probably already be obvious since there is no leading 'ffff'. > > > > We hash malformed specifiers also. Malformed specifiers include > > incomplete (e.g %pi) and also non-existent specifiers. checkpatch should > > warn for non-existent specifiers but AFAICT won't warn for incomplete > > specifiers. > > > > Here is the behaviour that this patch implements. > > > > For kpt_restrict==0 > > > > Randomness not ready: > > printed with %p: (pointer value) # NOTE: with padding > > Valid pointer: > > printed with %pK: deadbeefdeadbeef > > printed with %p: 00000000deadbeef > > malformed specifier (eg %i): 00000000deadbeef > > NULL pointer: > > printed with %pK: 0000000000000000 > > printed with %p: (null) # NOTE: with padding > > malformed specifier (eg %i): (null) > > > > For kpt_restrict==2 > > > > Valid pointer: > > printed with %pK: 0000000000000000 > > > > All other output as for kptr_restrict==0 > > > > V9: > > - Drop the initial patch from V8, leaving null pointer handling as is. > > - Print the hashed ID _without_ a '0x' suffix. > > - Mask the first 32 bits of the hashed ID to all zeros on 64 bit > > architectures. > > Oops, I had missed v9. This addresses my concerns. I think the leading > zeros are a good way to identify the "this is clearly not a kernel > address" issue (though the 32-bit folks may remain confused, but we > can fix that later, IMO). Awesome. Yeah this patch (coupled with the leaking_addresses.pl script) is turning out to be a bit 64-bit centric. However, as we plug more leaks in 64-bit kernels hopefully they will be plugged in 32-bit ones too. I can't think of any way to have leaking_addresses.pl grep for 32-bit addresses, especially once/if this patch gets merged. We will not be able to differentiate between hashed addresses and real addresses on 32-bit machines. thanks, Tobin.
On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote: > Currently there are many places in the kernel where addresses are being > printed using an unadorned %p. Kernel pointers should be printed using > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses > gives attackers sensitive information about the kernel layout in memory. > > We can reduce the attack surface by hashing all addresses printed with > %p. This will of course break some users, forcing code printing needed > addresses to be updated. I am sorry for my ignorance but what is the right update, please? I expect that there are several possibilities: + remove the pointer at all + replace it with %pK so that it honors kptr_restrict setting + any other option? Is kptr_restrict considered a safe mechanism? Also kptr_restrict seems to be primary for the messages that are available via /proc and /sys. Is it good enough for the messages logged by printk()? Will there be a debug option that would allow to see the original pointers? Or what is the preferred way for debug messages? > For what it's worth, usage of unadorned %p can be broken down as > follows (thanks to Joe Perches). > > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c > 1084 arch > 20 block > 10 crypto > 32 Documentation > 8121 drivers > 1221 fs > 143 include > 101 kernel > 69 lib > 100 mm > 1510 net > 40 samples > 7 scripts > 11 security > 166 sound > 152 tools > 2 virt It is evident that it will hit many people. I guess that they will be suprised and might have similar questions. It might make sense to decribe this in Documentation/printk-formats.txt. Best Regards, Petr
On Tue, Oct 31, 2017 at 04:39:44PM +0100, Petr Mladek wrote: > On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote: > > Currently there are many places in the kernel where addresses are being > > printed using an unadorned %p. Kernel pointers should be printed using > > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses > > gives attackers sensitive information about the kernel layout in memory. > > > > We can reduce the attack surface by hashing all addresses printed with > > %p. This will of course break some users, forcing code printing needed > > addresses to be updated. > > I am sorry for my ignorance but what is the right update, please? Can I say first that I am in no way an expert, I am new to both this problem and kernel dev in general. > I expect that there are several possibilities: > > + remove the pointer at all This definitely stops the leak! > + replace it with %pK so that it honors kptr_restrict setting I think this is the option of choice, see concerns below however. I get the feeling that the hope with this patch is that a vast majority of users of %p won't care so stopping all those addresses is the real win for this patch. The next hoped benefit is that the hashing will shed light on this topic and get developers to think about the issue before _wildly_ printing addresses. Having to work harder to print the address in future will aid this (assuming everyone doesn't just start using %x). > + any other option? Use %x or %X - really bad, this will create more pain in the future. > Is kptr_restrict considered a safe mechanism? > > Also kptr_restrict seems to be primary for the messages that are available > via /proc and /sys. Is it good enough for the messages logged by > printk()? There is some concern that kptr_restrict is not overly great. Linus is the main purveyor of this argument. I won't paraphrase here because I will not do the argument justice. See this thread for the whole discussion [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options Side note: If this (mentioning a previous thread) is bad form, or there is a better way to do it, could you please tell me. > Will there be a debug option that would allow to see the original > pointers? Or what is the preferred way for debug messages? It seems to me that there is a fundamental conflict between security and debugging here. To debug we need the addresses, for a secure kernel we need there to be _no_ addresses shown to user space. I don't think we have the final solution to this at the moment. > > For what it's worth, usage of unadorned %p can be broken down as > > follows (thanks to Joe Perches). > > > > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c > > 1084 arch > > 20 block > > 10 crypto > > 32 Documentation > > 8121 drivers > > 1221 fs > > 143 include > > 101 kernel > > 69 lib > > 100 mm > > 1510 net > > 40 samples > > 7 scripts > > 11 security > > 166 sound > > 152 tools > > 2 virt > > It is evident that it will hit many people. I guess that they will > be suprised and might have similar questions. It might make sense > to decribe this in Documentation/printk-formats.txt. Very good idea, V10 to include. thanks, Tobin.
Hi Tobin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc7 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tobin-C-Harding/printk-hash-addresses-printed-with-p/20171101-080718
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
lib/vsprintf.c: In function 'kernel_pointer':
>> lib/vsprintf.c:1359:10: error: 'kptr_restrict' undeclared (first use in this function)
switch (kptr_restrict) {
^~~~~~~~~~~~~
lib/vsprintf.c:1359:10: note: each undeclared identifier is reported only once for each function it appears in
vim +/kptr_restrict +1359 lib/vsprintf.c
1347
1348 static noinline_for_stack
1349 char *kernel_pointer(char *buf, char *end, const void *ptr,
1350 struct printf_spec spec)
1351 {
1352 spec.base = 16;
1353 spec.flags |= SMALL;
1354 if (spec.field_width == -1) {
1355 spec.field_width = 2 * sizeof(void *);
1356 spec.flags |= ZEROPAD;
1357 }
1358
> 1359 switch (kptr_restrict) {
1360 case 0:
1361 /* Always print %pK values */
1362 break;
1363 case 1: {
1364 const struct cred *cred;
1365
1366 /*
1367 * kptr_restrict==1 cannot be used in IRQ context
1368 * because its test for CAP_SYSLOG would be meaningless.
1369 */
1370 if (in_irq() || in_serving_softirq() || in_nmi())
1371 return string(buf, end, "pK-error", spec);
1372
1373 /*
1374 * Only print the real pointer value if the current
1375 * process has CAP_SYSLOG and is running with the
1376 * same credentials it started with. This is because
1377 * access to files is checked at open() time, but %pK
1378 * checks permission at read() time. We don't want to
1379 * leak pointer values if a binary opens a file using
1380 * %pK and then elevates privileges before reading it.
1381 */
1382 cred = current_cred();
1383 if (!has_capability_noaudit(current, CAP_SYSLOG) ||
1384 !uid_eq(cred->euid, cred->uid) ||
1385 !gid_eq(cred->egid, cred->gid))
1386 ptr = NULL;
1387 break;
1388 }
1389 case 2:
1390 default:
1391 /* Always print 0's for %pK */
1392 ptr = NULL;
1393 break;
1394 }
1395
1396 return number(buf, end, (unsigned long)ptr, spec);
1397 }
1398
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed 2017-11-01 10:53:45, Tobin C. Harding wrote: > On Tue, Oct 31, 2017 at 04:39:44PM +0100, Petr Mladek wrote: > > On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote: > > > Currently there are many places in the kernel where addresses are being > > > printed using an unadorned %p. Kernel pointers should be printed using > > > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses > > > gives attackers sensitive information about the kernel layout in memory. > > > > > > We can reduce the attack surface by hashing all addresses printed with > > > %p. This will of course break some users, forcing code printing needed > > > addresses to be updated. > > > > I am sorry for my ignorance but what is the right update, please? > > Can I say first that I am in no way an expert, I am new to both this > problem and kernel dev in general. Sure. There are many experienced people in CC. I hope that they might have some opinion on this. My concern is that this patch breaks some functionality because it is dangerous. This makes perfect sense. But people will want to fix it a safe way. And the way is not clear to me. > > I expect that there are several possibilities: > > > > + remove the pointer at all > > This definitely stops the leak! Sure. But this is not a solution for debugging tools, e.g. lockdep. sysrq-related dumps. Of course, the pointers must not be visible on production system to normal users but there should be a way to see them for debugging purposes. > > + replace it with %pK so that it honors kptr_restrict setting > > I think this is the option of choice, see concerns below however. I get > the feeling that the hope with this patch is that a vast majority of > users of %p won't care so stopping all those addresses is the real win > for this patch. > > The next hoped benefit is that the hashing will shed light on this topic > and get developers to think about the issue before _wildly_ printing > addresses. Having to work harder to print the address in future will aid > this (assuming everyone doesn't just start using %x). > > + any other option? > > Use %x or %X - really bad, this will create more pain in the future. Yes, using %x or %X is dangerous. It would be used by users that do not mind about security. Or is there any situation when always using the original pointer as a string is needed for a real functionality? IMHO, string is needed only for human readable usage. Then it always smells with information leak. > > Is kptr_restrict considered a safe mechanism? > > > > Also kptr_restrict seems to be primary for the messages that are available > > via /proc and /sys. Is it good enough for the messages logged by > > printk()? > > There is some concern that kptr_restrict is not overly great. Linus is > the main purveyor of this argument. I won't paraphrase here because I > will not do the argument justice. > > See this thread for the whole discussion > > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options I saw the discussion but it was a bit hard to follow. I would highlight the following three concerns related to kptr_restrict: First, it did not help to improve the situation. IMHO, this is mainly because it was opt-in and did not force people to fix the code. This is being addressed by this patch that actively breaks all users. Second, the user credentials are checked when the string is formatted. They do not reflect the path how the string is passed to the user. Therefore it might be cheated. This opens a question if kptr_restrict would stay in kernel and whether the conversion from %p to %pK is one of the proposed solutions. Third, kptr_restrict can be modified too late. It means that the pointers printed during the early boot will never or always be readable. It means that you would need two different kernel builds for production and debugging. Well, this is probably the smallest issue. All in all. I wonder if it would make sense to introduce some compiler or command line option that would allow to disable the hashing of pointers for debugging purposes. Best Regards, Petr
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..0c9a008fc256 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -33,6 +33,8 @@ #include <linux/uuid.h> #include <linux/of.h> #include <net/addrconf.h> +#include <linux/siphash.h> +#include <linux/compiler.h> #ifdef CONFIG_BLOCK #include <linux/blkdev.h> #endif @@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr, } static noinline_for_stack +char *kernel_pointer(char *buf, char *end, const void *ptr, + struct printf_spec spec) +{ + spec.base = 16; + spec.flags |= SMALL; + if (spec.field_width == -1) { + spec.field_width = 2 * sizeof(void *); + spec.flags |= ZEROPAD; + } + + switch (kptr_restrict) { + case 0: + /* Always print %pK values */ + break; + case 1: { + const struct cred *cred; + + /* + * kptr_restrict==1 cannot be used in IRQ context + * because its test for CAP_SYSLOG would be meaningless. + */ + if (in_irq() || in_serving_softirq() || in_nmi()) + return string(buf, end, "pK-error", spec); + + /* + * Only print the real pointer value if the current + * process has CAP_SYSLOG and is running with the + * same credentials it started with. This is because + * access to files is checked at open() time, but %pK + * checks permission at read() time. We don't want to + * leak pointer values if a binary opens a file using + * %pK and then elevates privileges before reading it. + */ + cred = current_cred(); + if (!has_capability_noaudit(current, CAP_SYSLOG) || + !uid_eq(cred->euid, cred->uid) || + !gid_eq(cred->egid, cred->gid)) + ptr = NULL; + break; + } + case 2: + default: + /* Always print 0's for %pK */ + ptr = NULL; + break; + } + + return number(buf, end, (unsigned long)ptr, spec); +} + +static noinline_for_stack char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt) { unsigned long long num; @@ -1591,6 +1644,66 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, return widen_string(buf, buf - buf_start, end, spec); } +static bool have_filled_random_ptr_key __read_mostly; +static siphash_key_t ptr_key __read_mostly; + +static void fill_random_ptr_key(struct random_ready_callback *unused) +{ + get_random_bytes(&ptr_key, sizeof(ptr_key)); + WRITE_ONCE(have_filled_random_ptr_key, true); +} + +static struct random_ready_callback random_ready = { + .func = fill_random_ptr_key +}; + +static int __init initialize_ptr_random(void) +{ + int ret = add_random_ready_callback(&random_ready); + + if (!ret) + return 0; + else if (ret == -EALREADY) { + fill_random_ptr_key(&random_ready); + return 0; + } + + return ret; +} +early_initcall(initialize_ptr_random); + +/* Maps a pointer to a 32 bit unique identifier. */ +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) +{ + unsigned long hashval; + const int default_width = 2 * sizeof(void *); + + if (unlikely(!have_filled_random_ptr_key)) { + spec.field_width = default_width; + return string(buf, end, "(pointer value)", spec); + } + +#ifdef CONFIG_64BIT + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key); + /* + * Mask off the first 32 bits, this makes explicit that we have + * modified the address (and 32 bits is plenty for a unique ID). + */ + hashval = hashval & 0xffffffff; +#else + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key); +#endif + + spec.flags |= SMALL; + if (spec.field_width == -1) { + spec.field_width = default_width; + spec.flags |= ZEROPAD; + } + spec.base = 16; + + return number(buf, end, hashval, spec); +} + int kptr_restrict __read_mostly; /* @@ -1703,6 +1816,9 @@ int kptr_restrict __read_mostly; * Note: The difference between 'S' and 'F' is that on ia64 and ppc64 * function pointers are really function descriptors, which contain a * pointer to the real address. + * + * Note: The default behaviour (unadorned %p) is to hash the address, + * rendering it useful as a unique identifier. */ static noinline_for_stack char *pointer(const char *fmt, char *buf, char *end, void *ptr, @@ -1792,47 +1908,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return buf; } case 'K': - switch (kptr_restrict) { - case 0: - /* Always print %pK values */ - break; - case 1: { - const struct cred *cred; - - /* - * kptr_restrict==1 cannot be used in IRQ context - * because its test for CAP_SYSLOG would be meaningless. - */ - if (in_irq() || in_serving_softirq() || in_nmi()) { - if (spec.field_width == -1) - spec.field_width = default_width; - return string(buf, end, "pK-error", spec); - } - - /* - * Only print the real pointer value if the current - * process has CAP_SYSLOG and is running with the - * same credentials it started with. This is because - * access to files is checked at open() time, but %pK - * checks permission at read() time. We don't want to - * leak pointer values if a binary opens a file using - * %pK and then elevates privileges before reading it. - */ - cred = current_cred(); - if (!has_capability_noaudit(current, CAP_SYSLOG) || - !uid_eq(cred->euid, cred->uid) || - !gid_eq(cred->egid, cred->gid)) - ptr = NULL; - break; - } - case 2: - default: - /* Always print 0's for %pK */ - ptr = NULL; - break; - } - break; - + return kernel_pointer(buf, end, ptr, spec); case 'N': return netdev_bits(buf, end, ptr, fmt); case 'a': @@ -1858,14 +1934,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return device_node_string(buf, end, ptr, spec, fmt + 1); } } - spec.flags |= SMALL; - if (spec.field_width == -1) { - spec.field_width = default_width; - spec.flags |= ZEROPAD; - } - spec.base = 16; - return number(buf, end, (unsigned long) ptr, spec); + /* default is to _not_ leak addresses, hash before printing */ + return ptr_to_id(buf, end, ptr, spec); } /*
Currently there are many places in the kernel where addresses are being printed using an unadorned %p. Kernel pointers should be printed using %pK allowing some control via the kptr_restrict sysctl. Exposing addresses gives attackers sensitive information about the kernel layout in memory. We can reduce the attack surface by hashing all addresses printed with %p. This will of course break some users, forcing code printing needed addresses to be updated. For what it's worth, usage of unadorned %p can be broken down as follows (thanks to Joe Perches). $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c 1084 arch 20 block 10 crypto 32 Documentation 8121 drivers 1221 fs 143 include 101 kernel 69 lib 100 mm 1510 net 40 samples 7 scripts 11 security 166 sound 152 tools 2 virt Add function ptr_to_id() to map an address to a 32 bit unique identifier. Hash any unadorned usage of specifier %p and any malformed specifiers. Signed-off-by: Tobin C. Harding <me@tobin.cc> --- It seems we don't have consensus on a couple of things 1. The size of the hashed address on 64 bit architectures. 2. The use of '0x' pre-fix for hashed addresses. In regards to (1), we are agreed that we only need 32 bits of information. There is some questions however that outputting _only_ 32 bits may break userland. In regards to (2), irrespective of the arguments for and against, if point 1 is correct and changing the format will break userland then we can't add the '0x' suffix for the same reason. Therefore this patch masks off the first 32 bits, retaining only 32 bits of information. We do not add a '0x' suffix. All in all, that results in _no_ change to the format of output only the content of the output. The leading 0's also make explicit that we have messed with the address, maybe this will save some debugging time by doing so. Although this would probably already be obvious since there is no leading 'ffff'. We hash malformed specifiers also. Malformed specifiers include incomplete (e.g %pi) and also non-existent specifiers. checkpatch should warn for non-existent specifiers but AFAICT won't warn for incomplete specifiers. Here is the behaviour that this patch implements. For kpt_restrict==0 Randomness not ready: printed with %p: (pointer value) # NOTE: with padding Valid pointer: printed with %pK: deadbeefdeadbeef printed with %p: 00000000deadbeef malformed specifier (eg %i): 00000000deadbeef NULL pointer: printed with %pK: 0000000000000000 printed with %p: (null) # NOTE: with padding malformed specifier (eg %i): (null) For kpt_restrict==2 Valid pointer: printed with %pK: 0000000000000000 All other output as for kptr_restrict==0 V9: - Drop the initial patch from V8, leaving null pointer handling as is. - Print the hashed ID _without_ a '0x' suffix. - Mask the first 32 bits of the hashed ID to all zeros on 64 bit architectures. V8: - Add second patch cleaning up null pointer printing in pointer() - Move %pK handling to separate function, further cleaning up pointer() - Move ptr_to_id() call outside of switch statement making hashing the default behaviour (including malformed specifiers). - Remove use of static_key, replace with simple boolean. V7: - Use tabs instead of spaces (ouch!). V6: - Use __early_initcall() to fill the SipHash key. - Use static keys to guard hashing before the key is available. V5: - Remove spin lock. - Add Jason A. Donenfeld to CC list by request. - Add Theodore Ts'o to CC list due to comment on previous version. V4: - Remove changes to siphash.{ch} - Do word size check, and return value cast, directly in ptr_to_id(). - Use add_ready_random_callback() to guard call to get_random_bytes() V3: - Use atomic_xchg() to guard setting [random] key. - Remove erroneous white space change. V2: - Use SipHash to do the hashing. The discussion related to this patch has been fragmented. There are three threads associated with this patch. Email threads by subject: [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options lib/vsprintf.c | 167 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 119 insertions(+), 48 deletions(-)