lkml.org 
[lkml]   [2017]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7] printk: hash addresses printed with %p
    On Wed, Oct 25, 2017 at 09:02:34PM +0200, Rasmus Villemoes wrote:
    > On 25 October 2017 at 01:57, Tobin C. Harding <me@tobin.cc> wrote:
    > > On Tue, Oct 24, 2017 at 09:25:20PM +0200, Rasmus Villemoes wrote:
    > >>
    > >> I haven't followed the discussion too closely, but has it been
    > >> considered to exempt NULL from hashing?
    > >
    > [snip]
    > >
    > > The code in question is;
    > >
    > > static noinline_for_stack
    > > char *pointer(const char *fmt, char *buf, char *end, void *ptr,
    > > struct printf_spec spec)
    > > {
    > > const int default_width = 2 * sizeof(void *);
    > >
    > > if (!ptr && *fmt != 'K') {
    > > /*
    > > * Print (null) with the same width as a pointer so it makes
    > > * tabular output look nice.
    > > */
    > > if (spec.field_width == -1)
    > > spec.field_width = default_width;
    > > return string(buf, end, "(null)", spec);
    > > }
    >
    > Ah, yes, I should have re-read the current code before commenting. So
    > we're effectively already exempting NULL due to this early handling.
    > Good, let's leave that.
    >
    > Regarding the tabular output: Ignore it, it's completely irrelevant to
    > the hardening efforts (good work, btw), and probably completely
    > irrelevant period. If anything I'd say the comment and the attempted
    > alignment should just be killed.

    Righto, I'm happy with that. Will add to next version.

    > > This check and print "(null)" is at the wrong level of abstraction. If we want tabular output to be
    > > correct for _all_ pointer specifiers then spec.field_width (for NULL) should be set to match whatever
    > > field_width is used in the associated output function. Removing the NULL check above would require
    > > NULL checks adding to at least;
    > >
    > > resource_string()
    > > bitmap_list_string()
    > > bitmap_string()
    > > mac_address_string()
    > > ip4_addr_string()
    > > ip4_addr_string_sa()
    > > ip6_addr_string_sa()
    > > uuid_string()
    > > netdev_bits()
    > > address_val()
    > > dentry_name()
    > > bdev_name()
    > > ptr_to_id()
    >
    > No, please don't. The NULL check makes perfect sense early in
    > pointer(), because many of these handlers would dereference the
    > pointer, and while it's probably a bug to pass NULL to say %pD, it's
    > obviously better to print (null) than crash. Adding NULL checks to
    > each of these is error-prone and lots of bloat for no real value
    > (consider that many of these can produce lots of variants of output,
    > and e.g. dotted-decimal ip addresses don't even have a well-defined
    > width at all).
    >
    > > My question is [snip] is it too trivial to matter?
    >
    > Yes.

    thanks,
    Tobin.

    \
     
     \ /
      Last update: 2017-10-26 00:15    [W:4.252 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site