Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Fri, Feb 9, 2018 at 2:03 PM, Petr Mladek  wrote:
> On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote:
>> On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
>> > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
>>
>> > To make it clear. I was talking about "%p" format that is handled
>> > in the pointer() function in lib/vsprintf.c. The "(null)" makes
>> > sense only for the many modifiers that do deference of
>> > the pointer, e.g. "%pa", "%pE", "%ph".
>>
>> JFYI: I have a patch to eliminate those for %pE & %ph.
>>
>> Google for "lib/vsprintf: Remove useless NULL checks" as a first in
>> the series for new extension to print times.
>
> I am slightly confused. IMHO, it makes sense to printk "(null)"
> for %pE and %ph.

Yes, but isn't it done by if (!ptr) check in the very beginning of the
pointer() helper?

> Or do you just want to avoid the duplicit check in hex_string()
> and escaped_string()?

And that is as well.

> Well, it might be better to discuss this once you send the patch.

I can Cc you, though the patch is pretty independent on the series. I
can send it separately.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Fri, Feb 9, 2018 at 2:03 PM, Petr Mladek  wrote:
> On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote:
>> On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
>> > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
>>
>> > To make it clear. I was talking about "%p" format that is handled
>> > in the pointer() function in lib/vsprintf.c. The "(null)" makes
>> > sense only for the many modifiers that do deference of
>> > the pointer, e.g. "%pa", "%pE", "%ph".
>>
>> JFYI: I have a patch to eliminate those for %pE & %ph.
>>
>> Google for "lib/vsprintf: Remove useless NULL checks" as a first in
>> the series for new extension to print times.
>
> I am slightly confused. IMHO, it makes sense to printk "(null)"
> for %pE and %ph.

Yes, but isn't it done by if (!ptr) check in the very beginning of the
pointer() helper?

> Or do you just want to avoid the duplicit check in hex_string()
> and escaped_string()?

And that is as well.

> Well, it might be better to discuss this once you send the patch.

I can Cc you, though the patch is pretty independent on the series. I
can send it separately.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote:
> On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
> > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
> 
> > To make it clear. I was talking about "%p" format that is handled
> > in the pointer() function in lib/vsprintf.c. The "(null)" makes
> > sense only for the many modifiers that do deference of
> > the pointer, e.g. "%pa", "%pE", "%ph".
> 
> JFYI: I have a patch to eliminate those for %pE & %ph.
>
> Google for "lib/vsprintf: Remove useless NULL checks" as a first in
> the series for new extension to print times.

I am slightly confused. IMHO, it makes sense to printk "(null)"
for %pE and %ph.

Or do you just want to avoid the duplicit check in hex_string()
and escaped_string()?

Well, it might be better to discuss this once you send the patch.

> I just need time to address comments and resend the series.

Good to know.

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote:
> On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
> > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
> 
> > To make it clear. I was talking about "%p" format that is handled
> > in the pointer() function in lib/vsprintf.c. The "(null)" makes
> > sense only for the many modifiers that do deference of
> > the pointer, e.g. "%pa", "%pE", "%ph".
> 
> JFYI: I have a patch to eliminate those for %pE & %ph.
>
> Google for "lib/vsprintf: Remove useless NULL checks" as a first in
> the series for new extension to print times.

I am slightly confused. IMHO, it makes sense to printk "(null)"
for %pE and %ph.

Or do you just want to avoid the duplicit check in hex_string()
and escaped_string()?

Well, it might be better to discuss this once you send the patch.

> I just need time to address comments and resend the series.

Good to know.

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
> On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:

> To make it clear. I was talking about "%p" format that is handled
> in the pointer() function in lib/vsprintf.c. The "(null)" makes
> sense only for the many modifiers that do deference of
> the pointer, e.g. "%pa", "%pE", "%ph".

JFYI: I have a patch to eliminate those for %pE & %ph.

Google for "lib/vsprintf: Remove useless NULL checks" as a first in
the series for new extension to print times.

I just need time to address comments and resend the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek  wrote:
> On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:

> To make it clear. I was talking about "%p" format that is handled
> in the pointer() function in lib/vsprintf.c. The "(null)" makes
> sense only for the many modifiers that do deference of
> the pointer, e.g. "%pa", "%pE", "%ph".

JFYI: I have a patch to eliminate those for %pE & %ph.

Google for "lib/vsprintf: Remove useless NULL checks" as a first in
the series for new extension to print times.

I just need time to address comments and resend the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Geert Uytterhoeven]

Hi Petr,

On Wed, Feb 7, 2018 at 4:41 PM, Petr Mladek  wrote:
> On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
>> On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
>> > [*] I made some archaeology:
>> >
>> > The "(null)" string was added by the commit d97106ab53f812910
>> > ("Make %p print '(null)' for NULL pointers").
>> >
>> > It was a generic solution to prevent eventual crashes, see
>> > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
>> >
>> > From this point, printing  for %px looks perfectly fine because
>> > it does not crash.
>> >
>> > In fact, it would have made perfect sense to print  for pure
>> > %p because it did not crash. But nobody has cared about the eventual
>> > confusion yet.
>> >
>> > I am not sure if it makes sense to change the pure %p handling
>> > now. Note that printing "(null)" has the advantage that we
>> > get this string instead of the hash ;-)
>>
>> Note that "(null)" is also used for printing strings, where you do 
>> dereference
>> the pointer, unlike for printing pointers.
>> In addition, "(null)" for strings is not just printed for real NULL
>> pointers, but
>> also for anything pointing within the first page of virtual memory.
>
> We are on the safe side. "(null)" for "%s" is handled
> separately, see string() function in lib/vsprintf.c.

I know.

> To make it clear. I was talking about "%p" format that is handled
> in the pointer() function in lib/vsprintf.c. The "(null)" makes
> sense only for the many modifiers that do deference of
> the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense
> for the pure "%p" used without any modifier. Well, it actually
> started to makes some sense after we started printing
> the hash instead of the real pointer value.

Sure. I agree with all of that.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Geert Uytterhoeven]

Hi Petr,

On Wed, Feb 7, 2018 at 4:41 PM, Petr Mladek  wrote:
> On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
>> On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
>> > [*] I made some archaeology:
>> >
>> > The "(null)" string was added by the commit d97106ab53f812910
>> > ("Make %p print '(null)' for NULL pointers").
>> >
>> > It was a generic solution to prevent eventual crashes, see
>> > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
>> >
>> > From this point, printing  for %px looks perfectly fine because
>> > it does not crash.
>> >
>> > In fact, it would have made perfect sense to print  for pure
>> > %p because it did not crash. But nobody has cared about the eventual
>> > confusion yet.
>> >
>> > I am not sure if it makes sense to change the pure %p handling
>> > now. Note that printing "(null)" has the advantage that we
>> > get this string instead of the hash ;-)
>>
>> Note that "(null)" is also used for printing strings, where you do 
>> dereference
>> the pointer, unlike for printing pointers.
>> In addition, "(null)" for strings is not just printed for real NULL
>> pointers, but
>> also for anything pointing within the first page of virtual memory.
>
> We are on the safe side. "(null)" for "%s" is handled
> separately, see string() function in lib/vsprintf.c.

I know.

> To make it clear. I was talking about "%p" format that is handled
> in the pointer() function in lib/vsprintf.c. The "(null)" makes
> sense only for the many modifiers that do deference of
> the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense
> for the pure "%p" used without any modifier. Well, it actually
> started to makes some sense after we started printing
> the hash instead of the real pointer value.

Sure. I agree with all of that.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
> Hi Petr,
> 
> On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
> > [*] I made some archaeology:
> >
> > The "(null)" string was added by the commit d97106ab53f812910
> > ("Make %p print '(null)' for NULL pointers").
> >
> > It was a generic solution to prevent eventual crashes, see
> > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
> >
> > From this point, printing  for %px looks perfectly fine because
> > it does not crash.
> >
> > In fact, it would have made perfect sense to print  for pure
> > %p because it did not crash. But nobody has cared about the eventual
> > confusion yet.
> >
> > I am not sure if it makes sense to change the pure %p handling
> > now. Note that printing "(null)" has the advantage that we
> > get this string instead of the hash ;-)
> 
> Note that "(null)" is also used for printing strings, where you do dereference
> the pointer, unlike for printing pointers.
> In addition, "(null)" for strings is not just printed for real NULL
> pointers, but
> also for anything pointing within the first page of virtual memory.

We are on the safe side. "(null)" for "%s" is handled
separately, see string() function in lib/vsprintf.c.

To make it clear. I was talking about "%p" format that is handled
in the pointer() function in lib/vsprintf.c. The "(null)" makes
sense only for the many modifiers that do deference of
the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense
for the pure "%p" used without any modifier. Well, it actually
started to makes some sense after we started printing
the hash instead of the real pointer value.

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote:
> Hi Petr,
> 
> On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
> > [*] I made some archaeology:
> >
> > The "(null)" string was added by the commit d97106ab53f812910
> > ("Make %p print '(null)' for NULL pointers").
> >
> > It was a generic solution to prevent eventual crashes, see
> > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
> >
> > From this point, printing  for %px looks perfectly fine because
> > it does not crash.
> >
> > In fact, it would have made perfect sense to print  for pure
> > %p because it did not crash. But nobody has cared about the eventual
> > confusion yet.
> >
> > I am not sure if it makes sense to change the pure %p handling
> > now. Note that printing "(null)" has the advantage that we
> > get this string instead of the hash ;-)
> 
> Note that "(null)" is also used for printing strings, where you do dereference
> the pointer, unlike for printing pointers.
> In addition, "(null)" for strings is not just printed for real NULL
> pointers, but
> also for anything pointing within the first page of virtual memory.

We are on the safe side. "(null)" for "%s" is handled
separately, see string() function in lib/vsprintf.c.

To make it clear. I was talking about "%p" format that is handled
in the pointer() function in lib/vsprintf.c. The "(null)" makes
sense only for the many modifiers that do deference of
the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense
for the pure "%p" used without any modifier. Well, it actually
started to makes some sense after we started printing
the hash instead of the real pointer value.

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Geert Uytterhoeven]

Hi Petr,

On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
> On Mon 2018-02-05 21:58:17, Adam Borowski wrote:
>> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
>> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
>> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
>> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
>> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> > >> >> Like %pK already does, print "" instead.
>> > >> >>
>> > >> >> This confused people -- the convention is that "(null)" means you 
>> > >> >> tried to
>> > >> >> dereference a null pointer as opposed to printing the address.
>> > >
>> > > Leaving aside what is converting to %px.  If we consider that using %px
>> > > is meant to convey to us that we _really_ want the address, in hex hence
>> > > the 'x', then it is not surprising that we will get ""'s for a
>> > > null pointer, right?  Yes it is different to before but since we are
>> > > changing the specifier does this not imply that there may be some
>> > > change?
>> >
>> > I personally prefer s, but if we're going to change this, we need
>> > to be aware of the difference.
>>
>> It's easy to paint this bikeshed any color you guys want to: there's an "if"
>> already.  My preference is also ; NULL would be good, too -- I just
>> don't want (null) as that has a special meaning in usual userspace
>> implementations; (null) also fits well most other modes of %p as they show
>> some object the argument points to.  Confusion = wasted debugging time.
>
>> Let's recap:
>>
>> Currently:
>>   not-null  null
>> %pponies  object's description  (null)
>> %px   address   (null)
>> %pK   hash  hash
>>
>> I'd propose:
>>   not-null  null
>> %pponies  object's description  (null)
>> %px   address   
>> %pK   hash  
>
> It makes sense to me[*], so
>
> Reviewed-by: Petr Mladek 
>
> It seems that all people agree with this change. But there was also some
> confusion. I am going to give it few more days before I push it to
> Linus. It means waiting for 4.16-rc3 because I will be without
> reliable internet next week. Anyone, feel free to push it faster.
>
>
> [*] I made some archaeology:
>
> The "(null)" string was added by the commit d97106ab53f812910
> ("Make %p print '(null)' for NULL pointers").
>
> It was a generic solution to prevent eventual crashes, see
> https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
>
> From this point, printing  for %px looks perfectly fine because
> it does not crash.
>
> In fact, it would have made perfect sense to print  for pure
> %p because it did not crash. But nobody has cared about the eventual
> confusion yet.
>
> I am not sure if it makes sense to change the pure %p handling
> now. Note that printing "(null)" has the advantage that we
> get this string instead of the hash ;-)

Note that "(null)" is also used for printing strings, where you do dereference
the pointer, unlike for printing pointers.
In addition, "(null)" for strings is not just printed for real NULL
pointers, but
also for anything pointing within the first page of virtual memory.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Geert Uytterhoeven]

Hi Petr,

On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek  wrote:
> On Mon 2018-02-05 21:58:17, Adam Borowski wrote:
>> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
>> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
>> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
>> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
>> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> > >> >> Like %pK already does, print "" instead.
>> > >> >>
>> > >> >> This confused people -- the convention is that "(null)" means you 
>> > >> >> tried to
>> > >> >> dereference a null pointer as opposed to printing the address.
>> > >
>> > > Leaving aside what is converting to %px.  If we consider that using %px
>> > > is meant to convey to us that we _really_ want the address, in hex hence
>> > > the 'x', then it is not surprising that we will get ""'s for a
>> > > null pointer, right?  Yes it is different to before but since we are
>> > > changing the specifier does this not imply that there may be some
>> > > change?
>> >
>> > I personally prefer s, but if we're going to change this, we need
>> > to be aware of the difference.
>>
>> It's easy to paint this bikeshed any color you guys want to: there's an "if"
>> already.  My preference is also ; NULL would be good, too -- I just
>> don't want (null) as that has a special meaning in usual userspace
>> implementations; (null) also fits well most other modes of %p as they show
>> some object the argument points to.  Confusion = wasted debugging time.
>
>> Let's recap:
>>
>> Currently:
>>   not-null  null
>> %pponies  object's description  (null)
>> %px   address   (null)
>> %pK   hash  hash
>>
>> I'd propose:
>>   not-null  null
>> %pponies  object's description  (null)
>> %px   address   
>> %pK   hash  
>
> It makes sense to me[*], so
>
> Reviewed-by: Petr Mladek 
>
> It seems that all people agree with this change. But there was also some
> confusion. I am going to give it few more days before I push it to
> Linus. It means waiting for 4.16-rc3 because I will be without
> reliable internet next week. Anyone, feel free to push it faster.
>
>
> [*] I made some archaeology:
>
> The "(null)" string was added by the commit d97106ab53f812910
> ("Make %p print '(null)' for NULL pointers").
>
> It was a generic solution to prevent eventual crashes, see
> https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org
>
> From this point, printing  for %px looks perfectly fine because
> it does not crash.
>
> In fact, it would have made perfect sense to print  for pure
> %p because it did not crash. But nobody has cared about the eventual
> confusion yet.
>
> I am not sure if it makes sense to change the pure %p handling
> now. Note that printing "(null)" has the advantage that we
> get this string instead of the hash ;-)

Note that "(null)" is also used for printing strings, where you do dereference
the pointer, unlike for printing pointers.
In addition, "(null)" for strings is not just printed for real NULL
pointers, but
also for anything pointing within the first page of virtual memory.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Mon 2018-02-05 21:58:17, Adam Borowski wrote:
> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > >> >> Like %pK already does, print "" instead.
> > >> >>
> > >> >> This confused people -- the convention is that "(null)" means you 
> > >> >> tried to
> > >> >> dereference a null pointer as opposed to printing the address.
> > >
> > > Leaving aside what is converting to %px.  If we consider that using %px
> > > is meant to convey to us that we _really_ want the address, in hex hence
> > > the 'x', then it is not surprising that we will get ""'s for a
> > > null pointer, right?  Yes it is different to before but since we are
> > > changing the specifier does this not imply that there may be some
> > > change?
> > 
> > I personally prefer s, but if we're going to change this, we need
> > to be aware of the difference.
> 
> It's easy to paint this bikeshed any color you guys want to: there's an "if"
> already.  My preference is also ; NULL would be good, too -- I just
> don't want (null) as that has a special meaning in usual userspace
> implementations; (null) also fits well most other modes of %p as they show
> some object the argument points to.  Confusion = wasted debugging time.

> Let's recap:
> 
> Currently:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   (null)
> %pK   hash  hash
> 
> I'd propose:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   
> %pK   hash  

It makes sense to me[*], so

Reviewed-by: Petr Mladek 

It seems that all people agree with this change. But there was also some
confusion. I am going to give it few more days before I push it to
Linus. It means waiting for 4.16-rc3 because I will be without
reliable internet next week. Anyone, feel free to push it faster.


[*] I made some archaeology:

The "(null)" string was added by the commit d97106ab53f812910
("Make %p print '(null)' for NULL pointers").

It was a generic solution to prevent eventual crashes, see
https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org

>From this point, printing  for %px looks perfectly fine because
it does not crash.

In fact, it would have made perfect sense to print  for pure
%p because it did not crash. But nobody has cared about the eventual
confusion yet.

I am not sure if it makes sense to change the pure %p handling
now. Note that printing "(null)" has the advantage that we
get this string instead of the hash ;-)

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

On Mon 2018-02-05 21:58:17, Adam Borowski wrote:
> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > >> >> Like %pK already does, print "" instead.
> > >> >>
> > >> >> This confused people -- the convention is that "(null)" means you 
> > >> >> tried to
> > >> >> dereference a null pointer as opposed to printing the address.
> > >
> > > Leaving aside what is converting to %px.  If we consider that using %px
> > > is meant to convey to us that we _really_ want the address, in hex hence
> > > the 'x', then it is not surprising that we will get ""'s for a
> > > null pointer, right?  Yes it is different to before but since we are
> > > changing the specifier does this not imply that there may be some
> > > change?
> > 
> > I personally prefer s, but if we're going to change this, we need
> > to be aware of the difference.
> 
> It's easy to paint this bikeshed any color you guys want to: there's an "if"
> already.  My preference is also ; NULL would be good, too -- I just
> don't want (null) as that has a special meaning in usual userspace
> implementations; (null) also fits well most other modes of %p as they show
> some object the argument points to.  Confusion = wasted debugging time.

> Let's recap:
> 
> Currently:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   (null)
> %pK   hash  hash
> 
> I'd propose:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   
> %pK   hash  

It makes sense to me[*], so

Reviewed-by: Petr Mladek 

It seems that all people agree with this change. But there was also some
confusion. I am going to give it few more days before I push it to
Linus. It means waiting for 4.16-rc3 because I will be without
reliable internet next week. Anyone, feel free to push it faster.


[*] I made some archaeology:

The "(null)" string was added by the commit d97106ab53f812910
("Make %p print '(null)' for NULL pointers").

It was a generic solution to prevent eventual crashes, see
https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xy...@speakeasy.org

>From this point, printing  for %px looks perfectly fine because
it does not crash.

In fact, it would have made perfect sense to print  for pure
%p because it did not crash. But nobody has cared about the eventual
confusion yet.

I am not sure if it makes sense to change the pure %p handling
now. Note that printing "(null)" has the advantage that we
get this string instead of the hash ;-)

Best Regards,
Petr

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Tue, Feb 6, 2018 at 12:22 AM, Tobin C. Harding  wrote:

> The original patch is good IMO and I AFAICT in everyone else's.

The original patch misses test cases.

Without them is problematic to follow what's going on with printing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Andy Shevchenko]

On Tue, Feb 6, 2018 at 12:22 AM, Tobin C. Harding  wrote:

> The original patch is good IMO and I AFAICT in everyone else's.

The original patch misses test cases.

Without them is problematic to follow what's going on with printing.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Roberts, William C]

> -Original Message-
> From: Tobin C. Harding [mailto:m...@tobin.cc]
> Sent: Monday, February 5, 2018 2:23 PM
> To: Adam Borowski <kilob...@angband.pl>
> Cc: Kees Cook <keesc...@chromium.org>; Petr Mladek <pmla...@suse.com>;
> Sergey Senozhatsky <sergey.senozhat...@gmail.com>; Steven Rostedt
> <rost...@goodmis.org>; LKML <linux-kernel@vger.kernel.org>; Andrew Morton
> <a...@linux-foundation.org>; Joe Perches <j...@perches.com>; Roberts,
> William C <william.c.robe...@intel.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; David Laight <david.lai...@aculab.com>; Randy Dunlap
> <rdun...@infradead.org>; Geert Uytterhoeven <ge...@linux-m68k.org>
> Subject: Re: [PATCH] vsprintf: avoid misleading "(null)" for %px
> 
> On Mon, Feb 05, 2018 at 09:58:17PM +0100, Adam Borowski wrote:
> > On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <m...@tobin.cc> wrote:
> > > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmla...@suse.com>
> wrote:
> > > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > > >> >> Like %pK already does, print "" instead.
> > > >> >>
> > > >> >> This confused people -- the convention is that "(null)" means
> > > >> >> you tried to dereference a null pointer as opposed to printing the
> address.
> > > >
> > > > Leaving aside what is converting to %px.  If we consider that
> > > > using %px is meant to convey to us that we _really_ want the
> > > > address, in hex hence the 'x', then it is not surprising that we
> > > > will get ""'s for a null pointer, right?  Yes it is
> > > > different to before but since we are changing the specifier does
> > > > this not imply that there may be some change?
> > >
> > > I personally prefer s, but if we're going to change this, we
> > > need to be aware of the difference.
> >
> > It's easy to paint this bikeshed any color you guys want to: there's an "if"
> > already.  My preference is also ; NULL would be good, too -- I
> > just don't want (null) as that has a special meaning in usual
> > userspace implementations; (null) also fits well most other modes of
> > %p as they show some object the argument points to.  Confusion = wasted
> debugging time.
> >
> > This is consistent with what we had before, with %pK special-cased.
> >
> > > > In what is now to be expected fashion for %p the discussion
> > > > appears to have split into two different things - what to do with
> > > > %px and what to do with %pK :)
> > >
> > > I say leave %pK alone. :)
> >
> > As in, printing some random (hashed) value?
> >
> >
> > Let's recap:
> >
> > Currently:
> >   not-null  null
> > %pponies  object's description  (null)
> > %px   address   (null)
> > %pK   hash  hash
> >
> > I'd propose:
> >   not-null  null
> > %pponies  object's description  (null)
> > %px   address   
> > %pK   hash  
> >
> > The initial patch in this thread changes printk("%px",0) from (null)
> > to ; what Tobin complained about is that printk("%pK",0) prints a
> > random value.
> 
> Epic fail on my behalf, my first comment was _wrong_ and brought %pK into the
> discussion - bad Tobin, please crawl back under your rock.
> 
> The original patch is good IMO and I AFAICT in everyone else's.
Nod.
> 
>   Tobin

RE: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Roberts, William C]

> -Original Message-
> From: Tobin C. Harding [mailto:m...@tobin.cc]
> Sent: Monday, February 5, 2018 2:23 PM
> To: Adam Borowski 
> Cc: Kees Cook ; Petr Mladek ;
> Sergey Senozhatsky ; Steven Rostedt
> ; LKML ; Andrew Morton
> ; Joe Perches ; Roberts,
> William C ; Linus Torvalds  foundation.org>; David Laight ; Randy Dunlap
> ; Geert Uytterhoeven 
> Subject: Re: [PATCH] vsprintf: avoid misleading "(null)" for %px
> 
> On Mon, Feb 05, 2018 at 09:58:17PM +0100, Adam Borowski wrote:
> > On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek 
> wrote:
> > > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > > >> >> Like %pK already does, print "" instead.
> > > >> >>
> > > >> >> This confused people -- the convention is that "(null)" means
> > > >> >> you tried to dereference a null pointer as opposed to printing the
> address.
> > > >
> > > > Leaving aside what is converting to %px.  If we consider that
> > > > using %px is meant to convey to us that we _really_ want the
> > > > address, in hex hence the 'x', then it is not surprising that we
> > > > will get ""'s for a null pointer, right?  Yes it is
> > > > different to before but since we are changing the specifier does
> > > > this not imply that there may be some change?
> > >
> > > I personally prefer s, but if we're going to change this, we
> > > need to be aware of the difference.
> >
> > It's easy to paint this bikeshed any color you guys want to: there's an "if"
> > already.  My preference is also ; NULL would be good, too -- I
> > just don't want (null) as that has a special meaning in usual
> > userspace implementations; (null) also fits well most other modes of
> > %p as they show some object the argument points to.  Confusion = wasted
> debugging time.
> >
> > This is consistent with what we had before, with %pK special-cased.
> >
> > > > In what is now to be expected fashion for %p the discussion
> > > > appears to have split into two different things - what to do with
> > > > %px and what to do with %pK :)
> > >
> > > I say leave %pK alone. :)
> >
> > As in, printing some random (hashed) value?
> >
> >
> > Let's recap:
> >
> > Currently:
> >   not-null  null
> > %pponies  object's description  (null)
> > %px   address   (null)
> > %pK   hash  hash
> >
> > I'd propose:
> >   not-null  null
> > %pponies  object's description  (null)
> > %px   address   
> > %pK   hash  
> >
> > The initial patch in this thread changes printk("%px",0) from (null)
> > to ; what Tobin complained about is that printk("%pK",0) prints a
> > random value.
> 
> Epic fail on my behalf, my first comment was _wrong_ and brought %pK into the
> discussion - bad Tobin, please crawl back under your rock.
> 
> The original patch is good IMO and I AFAICT in everyone else's.
Nod.
> 
>   Tobin

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 09:58:17PM +0100, Adam Borowski wrote:
> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > >> >> Like %pK already does, print "" instead.
> > >> >>
> > >> >> This confused people -- the convention is that "(null)" means you 
> > >> >> tried to
> > >> >> dereference a null pointer as opposed to printing the address.
> > >
> > > Leaving aside what is converting to %px.  If we consider that using %px
> > > is meant to convey to us that we _really_ want the address, in hex hence
> > > the 'x', then it is not surprising that we will get ""'s for a
> > > null pointer, right?  Yes it is different to before but since we are
> > > changing the specifier does this not imply that there may be some
> > > change?
> > 
> > I personally prefer s, but if we're going to change this, we need
> > to be aware of the difference.
> 
> It's easy to paint this bikeshed any color you guys want to: there's an "if"
> already.  My preference is also ; NULL would be good, too -- I just
> don't want (null) as that has a special meaning in usual userspace
> implementations; (null) also fits well most other modes of %p as they show
> some object the argument points to.  Confusion = wasted debugging time.
> 
> This is consistent with what we had before, with %pK special-cased.
> 
> > > In what is now to be expected fashion for %p the discussion appears to
> > > have split into two different things - what to do with %px and what to
> > > do with %pK :)
> > 
> > I say leave %pK alone. :)
> 
> As in, printing some random (hashed) value?
> 
> 
> Let's recap:
> 
> Currently:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   (null)
> %pK   hash  hash
> 
> I'd propose:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   
> %pK   hash  
> 
> The initial patch in this thread changes printk("%px",0) from (null) to
> ; what Tobin complained about is that printk("%pK",0) prints a
> random value.   

Epic fail on my behalf, my first comment was _wrong_ and brought %pK
into the discussion - bad Tobin, please crawl back under your rock.

The original patch is good IMO and I AFAICT in everyone else's.

Tobin

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 09:58:17PM +0100, Adam Borowski wrote:
> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > >> >> Like %pK already does, print "" instead.
> > >> >>
> > >> >> This confused people -- the convention is that "(null)" means you 
> > >> >> tried to
> > >> >> dereference a null pointer as opposed to printing the address.
> > >
> > > Leaving aside what is converting to %px.  If we consider that using %px
> > > is meant to convey to us that we _really_ want the address, in hex hence
> > > the 'x', then it is not surprising that we will get ""'s for a
> > > null pointer, right?  Yes it is different to before but since we are
> > > changing the specifier does this not imply that there may be some
> > > change?
> > 
> > I personally prefer s, but if we're going to change this, we need
> > to be aware of the difference.
> 
> It's easy to paint this bikeshed any color you guys want to: there's an "if"
> already.  My preference is also ; NULL would be good, too -- I just
> don't want (null) as that has a special meaning in usual userspace
> implementations; (null) also fits well most other modes of %p as they show
> some object the argument points to.  Confusion = wasted debugging time.
> 
> This is consistent with what we had before, with %pK special-cased.
> 
> > > In what is now to be expected fashion for %p the discussion appears to
> > > have split into two different things - what to do with %px and what to
> > > do with %pK :)
> > 
> > I say leave %pK alone. :)
> 
> As in, printing some random (hashed) value?
> 
> 
> Let's recap:
> 
> Currently:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   (null)
> %pK   hash  hash
> 
> I'd propose:
>   not-null  null
> %pponies  object's description  (null)
> %px   address   
> %pK   hash  
> 
> The initial patch in this thread changes printk("%px",0) from (null) to
> ; what Tobin complained about is that printk("%pK",0) prints a
> random value.   

Epic fail on my behalf, my first comment was _wrong_ and brought %pK
into the discussion - bad Tobin, please crawl back under your rock.

The original patch is good IMO and I AFAICT in everyone else's.

Tobin

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Adam Borowski]

On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> >> >> Like %pK already does, print "" instead.
> >> >>
> >> >> This confused people -- the convention is that "(null)" means you tried 
> >> >> to
> >> >> dereference a null pointer as opposed to printing the address.
> >
> > Leaving aside what is converting to %px.  If we consider that using %px
> > is meant to convey to us that we _really_ want the address, in hex hence
> > the 'x', then it is not surprising that we will get ""'s for a
> > null pointer, right?  Yes it is different to before but since we are
> > changing the specifier does this not imply that there may be some
> > change?
> 
> I personally prefer s, but if we're going to change this, we need
> to be aware of the difference.

It's easy to paint this bikeshed any color you guys want to: there's an "if"
already.  My preference is also ; NULL would be good, too -- I just
don't want (null) as that has a special meaning in usual userspace
implementations; (null) also fits well most other modes of %p as they show
some object the argument points to.  Confusion = wasted debugging time.

This is consistent with what we had before, with %pK special-cased.

> > In what is now to be expected fashion for %p the discussion appears to
> > have split into two different things - what to do with %px and what to
> > do with %pK :)
> 
> I say leave %pK alone. :)

As in, printing some random (hashed) value?


Let's recap:

Currently:
  not-null  null
%pponies  object's description  (null)
%px   address   (null)
%pK   hash  hash

I'd propose:
  not-null  null
%pponies  object's description  (null)
%px   address   
%pK   hash  

The initial patch in this thread changes printk("%px",0) from (null) to
; what Tobin complained about is that printk("%pK",0) prints a
random value.   


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration
⣾⠁⢰⠒⠀⣿⡁ camps is back.  What about KL Warschau (operating until 1956)?
⢿⡄⠘⠷⠚⠋⠀ Zgoda?  Łambinowice?  Most ex-German KLs?  If those were "soviet
⠈⠳⣄ puppets", Bereza Kartuska?  Sikorski's camps in UK (thanks Brits!)?

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Adam Borowski]

On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote:
> On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> >> >> Like %pK already does, print "" instead.
> >> >>
> >> >> This confused people -- the convention is that "(null)" means you tried 
> >> >> to
> >> >> dereference a null pointer as opposed to printing the address.
> >
> > Leaving aside what is converting to %px.  If we consider that using %px
> > is meant to convey to us that we _really_ want the address, in hex hence
> > the 'x', then it is not surprising that we will get ""'s for a
> > null pointer, right?  Yes it is different to before but since we are
> > changing the specifier does this not imply that there may be some
> > change?
> 
> I personally prefer s, but if we're going to change this, we need
> to be aware of the difference.

It's easy to paint this bikeshed any color you guys want to: there's an "if"
already.  My preference is also ; NULL would be good, too -- I just
don't want (null) as that has a special meaning in usual userspace
implementations; (null) also fits well most other modes of %p as they show
some object the argument points to.  Confusion = wasted debugging time.

This is consistent with what we had before, with %pK special-cased.

> > In what is now to be expected fashion for %p the discussion appears to
> > have split into two different things - what to do with %px and what to
> > do with %pK :)
> 
> I say leave %pK alone. :)

As in, printing some random (hashed) value?


Let's recap:

Currently:
  not-null  null
%pponies  object's description  (null)
%px   address   (null)
%pK   hash  hash

I'd propose:
  not-null  null
%pponies  object's description  (null)
%px   address   
%pK   hash  

The initial patch in this thread changes printk("%px",0) from (null) to
; what Tobin complained about is that printk("%pK",0) prints a
random value.   


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration
⣾⠁⢰⠒⠀⣿⡁ camps is back.  What about KL Warschau (operating until 1956)?
⢿⡄⠘⠷⠚⠋⠀ Zgoda?  Łambinowice?  Most ex-German KLs?  If those were "soviet
⠈⠳⣄ puppets", Bereza Kartuska?  Sikorski's camps in UK (thanks Brits!)?

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Kees Cook]

On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
>> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
>> > Hi,
>> >
>> > I add people who actively commented on adding %px modifier,
>> > see the thread starting at
>> > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
>> >
>> > Just for reference. It seems to be related to the commit 9f36e2c448007b54
>> > ("printk: use %pK for /proc/kallsyms and /proc/modules").
>> >
>> >
>> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> >> Like %pK already does, print "" instead.
>> >>
>> >> This confused people -- the convention is that "(null)" means you tried to
>> >> dereference a null pointer as opposed to printing the address.
>> >
>> > By other words, this avoids regressions when people convert
>> > %x to %px. Do I get it right, please?
>>
>> Nothing should be converting from %x to %px, it's %p to %px. %p print
>> "(null)" for 0x0, so it would be surprising for a conversion from %p
>> to %px to change that. (Though generally speaking "(null)" is never
>> useful...)
>
> Leaving aside what is converting to %px.  If we consider that using %px
> is meant to convey to us that we _really_ want the address, in hex hence
> the 'x', then it is not surprising that we will get ""'s for a
> null pointer, right?  Yes it is different to before but since we are
> changing the specifier does this not imply that there may be some
> change?

I personally prefer s, but if we're going to change this, we need
to be aware of the difference.

> In what is now to be expected fashion for %p the discussion appears to
> have split into two different things - what to do with %px and what to
> do with %pK :)

I say leave %pK alone. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Kees Cook]

On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding  wrote:
> On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
>> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
>> > Hi,
>> >
>> > I add people who actively commented on adding %px modifier,
>> > see the thread starting at
>> > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
>> >
>> > Just for reference. It seems to be related to the commit 9f36e2c448007b54
>> > ("printk: use %pK for /proc/kallsyms and /proc/modules").
>> >
>> >
>> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> >> Like %pK already does, print "" instead.
>> >>
>> >> This confused people -- the convention is that "(null)" means you tried to
>> >> dereference a null pointer as opposed to printing the address.
>> >
>> > By other words, this avoids regressions when people convert
>> > %x to %px. Do I get it right, please?
>>
>> Nothing should be converting from %x to %px, it's %p to %px. %p print
>> "(null)" for 0x0, so it would be surprising for a conversion from %p
>> to %px to change that. (Though generally speaking "(null)" is never
>> useful...)
>
> Leaving aside what is converting to %px.  If we consider that using %px
> is meant to convey to us that we _really_ want the address, in hex hence
> the 'x', then it is not surprising that we will get ""'s for a
> null pointer, right?  Yes it is different to before but since we are
> changing the specifier does this not imply that there may be some
> change?

I personally prefer s, but if we're going to change this, we need
to be aware of the difference.

> In what is now to be expected fashion for %p the discussion appears to
> have split into two different things - what to do with %px and what to
> do with %pK :)

I say leave %pK alone. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 09:36:03AM -0800, Randy Dunlap wrote:
> On 02/05/2018 08:49 AM, Steven Rostedt wrote:
> > On Mon, 5 Feb 2018 16:22:19 +0100
> > Adam Borowski  wrote:
> > 
> >> My change touches %px only, where your concern doesn't apply.
> >>
> >> You're right, though, when it comes to %pK:
> >> printk("%%pK: %pK, %%px: %px\n", 0, 0);
> >> says
> >> %pK: ba8bdc0a, %px: 
> >>
> >> So what should we do?  Avoid hashing 0?  Print a special value?
> > 
> > My personal opinion is that NULL should stay NULL and not be hashed.
> > What security issue could be leaked by a NULL? I'm not a security
> > person, that's a real question.
> 
> Agree.

While these views seem valid I don't think we are going to get much
love trying to change %pK to give a smidgen more information when %pK is
arguably out of favour :)

Tobin

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 09:36:03AM -0800, Randy Dunlap wrote:
> On 02/05/2018 08:49 AM, Steven Rostedt wrote:
> > On Mon, 5 Feb 2018 16:22:19 +0100
> > Adam Borowski  wrote:
> > 
> >> My change touches %px only, where your concern doesn't apply.
> >>
> >> You're right, though, when it comes to %pK:
> >> printk("%%pK: %pK, %%px: %px\n", 0, 0);
> >> says
> >> %pK: ba8bdc0a, %px: 
> >>
> >> So what should we do?  Avoid hashing 0?  Print a special value?
> > 
> > My personal opinion is that NULL should stay NULL and not be hashed.
> > What security issue could be leaked by a NULL? I'm not a security
> > person, that's a real question.
> 
> Agree.

While these views seem valid I don't think we are going to get much
love trying to change %pK to give a smidgen more information when %pK is
arguably out of favour :)

Tobin

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > Hi,
> >
> > I add people who actively commented on adding %px modifier,
> > see the thread starting at
> > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
> >
> > Just for reference. It seems to be related to the commit 9f36e2c448007b54
> > ("printk: use %pK for /proc/kallsyms and /proc/modules").
> >
> >
> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> >> Like %pK already does, print "" instead.
> >>
> >> This confused people -- the convention is that "(null)" means you tried to
> >> dereference a null pointer as opposed to printing the address.
> >
> > By other words, this avoids regressions when people convert
> > %x to %px. Do I get it right, please?
> 
> Nothing should be converting from %x to %px, it's %p to %px. %p print
> "(null)" for 0x0, so it would be surprising for a conversion from %p
> to %px to change that. (Though generally speaking "(null)" is never
> useful...)

Leaving aside what is converting to %px.  If we consider that using %px
is meant to convey to us that we _really_ want the address, in hex hence
the 'x', then it is not surprising that we will get ""'s for a
null pointer, right?  Yes it is different to before but since we are
changing the specifier does this not imply that there may be some
change?

In what is now to be expected fashion for %p the discussion appears to
have split into two different things - what to do with %px and what to
do with %pK :)

thanks,
Tobin.

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote:
> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> > Hi,
> >
> > I add people who actively commented on adding %px modifier,
> > see the thread starting at
> > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
> >
> > Just for reference. It seems to be related to the commit 9f36e2c448007b54
> > ("printk: use %pK for /proc/kallsyms and /proc/modules").
> >
> >
> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> >> Like %pK already does, print "" instead.
> >>
> >> This confused people -- the convention is that "(null)" means you tried to
> >> dereference a null pointer as opposed to printing the address.
> >
> > By other words, this avoids regressions when people convert
> > %x to %px. Do I get it right, please?
> 
> Nothing should be converting from %x to %px, it's %p to %px. %p print
> "(null)" for 0x0, so it would be surprising for a conversion from %p
> to %px to change that. (Though generally speaking "(null)" is never
> useful...)

Leaving aside what is converting to %px.  If we consider that using %px
is meant to convey to us that we _really_ want the address, in hex hence
the 'x', then it is not surprising that we will get ""'s for a
null pointer, right?  Yes it is different to before but since we are
changing the specifier does this not imply that there may be some
change?

In what is now to be expected fashion for %p the discussion appears to
have split into two different things - what to do with %px and what to
do with %pK :)

thanks,
Tobin.

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Kees Cook]

On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> Hi,
>
> I add people who actively commented on adding %px modifier,
> see the thread starting at
> https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
>
> Just for reference. It seems to be related to the commit 9f36e2c448007b54
> ("printk: use %pK for /proc/kallsyms and /proc/modules").
>
>
> On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> Like %pK already does, print "" instead.
>>
>> This confused people -- the convention is that "(null)" means you tried to
>> dereference a null pointer as opposed to printing the address.
>
> By other words, this avoids regressions when people convert
> %x to %px. Do I get it right, please?

Nothing should be converting from %x to %px, it's %p to %px. %p print
"(null)" for 0x0, so it would be surprising for a conversion from %p
to %px to change that. (Though generally speaking "(null)" is never
useful...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Kees Cook]

On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek  wrote:
> Hi,
>
> I add people who actively commented on adding %px modifier,
> see the thread starting at
> https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
>
> Just for reference. It seems to be related to the commit 9f36e2c448007b54
> ("printk: use %pK for /proc/kallsyms and /proc/modules").
>
>
> On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
>> Like %pK already does, print "" instead.
>>
>> This confused people -- the convention is that "(null)" means you tried to
>> dereference a null pointer as opposed to printing the address.
>
> By other words, this avoids regressions when people convert
> %x to %px. Do I get it right, please?

Nothing should be converting from %x to %px, it's %p to %px. %p print
"(null)" for 0x0, so it would be surprising for a conversion from %p
to %px to change that. (Though generally speaking "(null)" is never
useful...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Randy Dunlap]

On 02/05/2018 08:49 AM, Steven Rostedt wrote:
> On Mon, 5 Feb 2018 16:22:19 +0100
> Adam Borowski  wrote:
> 
>> My change touches %px only, where your concern doesn't apply.
>>
>> You're right, though, when it comes to %pK:
>> printk("%%pK: %pK, %%px: %px\n", 0, 0);
>> says
>> %pK: ba8bdc0a, %px: 
>>
>> So what should we do?  Avoid hashing 0?  Print a special value?
> 
> My personal opinion is that NULL should stay NULL and not be hashed.
> What security issue could be leaked by a NULL? I'm not a security
> person, that's a real question.

Agree.


-- 
~Randy

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Randy Dunlap]

On 02/05/2018 08:49 AM, Steven Rostedt wrote:
> On Mon, 5 Feb 2018 16:22:19 +0100
> Adam Borowski  wrote:
> 
>> My change touches %px only, where your concern doesn't apply.
>>
>> You're right, though, when it comes to %pK:
>> printk("%%pK: %pK, %%px: %px\n", 0, 0);
>> says
>> %pK: ba8bdc0a, %px: 
>>
>> So what should we do?  Avoid hashing 0?  Print a special value?
> 
> My personal opinion is that NULL should stay NULL and not be hashed.
> What security issue could be leaked by a NULL? I'm not a security
> person, that's a real question.

Agree.


-- 
~Randy

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Steven Rostedt]

On Mon, 5 Feb 2018 16:22:19 +0100
Adam Borowski  wrote:

> My change touches %px only, where your concern doesn't apply.
> 
> You're right, though, when it comes to %pK:
> printk("%%pK: %pK, %%px: %px\n", 0, 0);
> says
> %pK: ba8bdc0a, %px: 
> 
> So what should we do?  Avoid hashing 0?  Print a special value?

My personal opinion is that NULL should stay NULL and not be hashed.
What security issue could be leaked by a NULL? I'm not a security
person, that's a real question.

-- Steve

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Steven Rostedt]

On Mon, 5 Feb 2018 16:22:19 +0100
Adam Borowski  wrote:

> My change touches %px only, where your concern doesn't apply.
> 
> You're right, though, when it comes to %pK:
> printk("%%pK: %pK, %%px: %px\n", 0, 0);
> says
> %pK: ba8bdc0a, %px: 
> 
> So what should we do?  Avoid hashing 0?  Print a special value?

My personal opinion is that NULL should stay NULL and not be hashed.
What security issue could be leaked by a NULL? I'm not a security
person, that's a real question.

-- Steve

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Adam Borowski]

On Mon, Feb 05, 2018 at 09:03:05PM +1100, Tobin C. Harding wrote:
> On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote:
> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > > Like %pK already does, print "" instead.
> > >
> > > This confused people -- the convention is that "(null)" means you tried to
> > > dereference a null pointer as opposed to printing the address.
> > 
> > By other words, this avoids regressions when people convert
> > %x to %px. Do I get it right, please?

It's a regression in the sense that it confuses people.  %px never could
dereference a pointer so the information provided doesn't change, merely its
presentation.

> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 77ee6ced11b1..d7a708f82559 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char 
> > > *end, void *ptr,
> > >  {
> > >   const int default_width = 2 * sizeof(void *);
> > >  
> > > - if (!ptr && *fmt != 'K') {
> > > + if (!ptr && *fmt != 'K' && *fmt != 'x') {
> 
> I don't know if it matters but with this it won't be immediately
> apparent that a null pointer was printed (since zero could hash to
> anything).

My change touches %px only, where your concern doesn't apply.

You're right, though, when it comes to %pK:
printk("%%pK: %pK, %%px: %px\n", 0, 0);
says
%pK: ba8bdc0a, %px: 

So what should we do?  Avoid hashing 0?  Print a special value?


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration
⣾⠁⢰⠒⠀⣿⡁ camps is back.  What about KL Warschau (operating until 1956)?
⢿⡄⠘⠷⠚⠋⠀ Zgoda?  Łambinowice?  Most ex-German KLs?  If those were "soviet
⠈⠳⣄ puppets", Bereza Kartuska?  Sikorski's camps in UK (thanks Brits!)?

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Adam Borowski]

On Mon, Feb 05, 2018 at 09:03:05PM +1100, Tobin C. Harding wrote:
> On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote:
> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > > Like %pK already does, print "" instead.
> > >
> > > This confused people -- the convention is that "(null)" means you tried to
> > > dereference a null pointer as opposed to printing the address.
> > 
> > By other words, this avoids regressions when people convert
> > %x to %px. Do I get it right, please?

It's a regression in the sense that it confuses people.  %px never could
dereference a pointer so the information provided doesn't change, merely its
presentation.

> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 77ee6ced11b1..d7a708f82559 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char 
> > > *end, void *ptr,
> > >  {
> > >   const int default_width = 2 * sizeof(void *);
> > >  
> > > - if (!ptr && *fmt != 'K') {
> > > + if (!ptr && *fmt != 'K' && *fmt != 'x') {
> 
> I don't know if it matters but with this it won't be immediately
> apparent that a null pointer was printed (since zero could hash to
> anything).

My change touches %px only, where your concern doesn't apply.

You're right, though, when it comes to %pK:
printk("%%pK: %pK, %%px: %px\n", 0, 0);
says
%pK: ba8bdc0a, %px: 

So what should we do?  Avoid hashing 0?  Print a special value?


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration
⣾⠁⢰⠒⠀⣿⡁ camps is back.  What about KL Warschau (operating until 1956)?
⢿⡄⠘⠷⠚⠋⠀ Zgoda?  Łambinowice?  Most ex-German KLs?  If those were "soviet
⠈⠳⣄ puppets", Bereza Kartuska?  Sikorski's camps in UK (thanks Brits!)?

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote:
> Hi,
> 
> I add people who actively commented on adding %px modifier,
> see the thread starting at
> https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
> 
> Just for reference. It seems to be related to the commit 9f36e2c448007b54
> ("printk: use %pK for /proc/kallsyms and /proc/modules").
> 
> 
> On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > Like %pK already does, print "" instead.
> >
> > This confused people -- the convention is that "(null)" means you tried to
> > dereference a null pointer as opposed to printing the address.
> 
> By other words, this avoids regressions when people convert
> %x to %px. Do I get it right, please?
>
> > Signed-off-by: Adam Borowski 
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 77ee6ced11b1..d7a708f82559 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, 
> > void *ptr,
> >  {
> > const int default_width = 2 * sizeof(void *);
> >  
> > -   if (!ptr && *fmt != 'K') {
> > +   if (!ptr && *fmt != 'K' && *fmt != 'x') {

I don't know if it matters but with this it won't be immediately
apparent that a null pointer was printed (since zero could hash to
anything).

thanks,
Tobin.

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Tobin C. Harding]

On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote:
> Hi,
> 
> I add people who actively commented on adding %px modifier,
> see the thread starting at
> https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc
> 
> Just for reference. It seems to be related to the commit 9f36e2c448007b54
> ("printk: use %pK for /proc/kallsyms and /proc/modules").
> 
> 
> On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> > Like %pK already does, print "" instead.
> >
> > This confused people -- the convention is that "(null)" means you tried to
> > dereference a null pointer as opposed to printing the address.
> 
> By other words, this avoids regressions when people convert
> %x to %px. Do I get it right, please?
>
> > Signed-off-by: Adam Borowski 
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 77ee6ced11b1..d7a708f82559 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, 
> > void *ptr,
> >  {
> > const int default_width = 2 * sizeof(void *);
> >  
> > -   if (!ptr && *fmt != 'K') {
> > +   if (!ptr && *fmt != 'K' && *fmt != 'x') {

I don't know if it matters but with this it won't be immediately
apparent that a null pointer was printed (since zero could hash to
anything).

thanks,
Tobin.

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

Hi,

I add people who actively commented on adding %px modifier,
see the thread starting at
https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc

Just for reference. It seems to be related to the commit 9f36e2c448007b54
("printk: use %pK for /proc/kallsyms and /proc/modules").


On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> Like %pK already does, print "" instead.
>
> This confused people -- the convention is that "(null)" means you tried to
> dereference a null pointer as opposed to printing the address.

By other words, this avoids regressions when people convert
%x to %px. Do I get it right, please?

> Signed-off-by: Adam Borowski 
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 77ee6ced11b1..d7a708f82559 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, 
> void *ptr,
>  {
>   const int default_width = 2 * sizeof(void *);
>  
> - if (!ptr && *fmt != 'K') {
> + if (!ptr && *fmt != 'K' && *fmt != 'x') {
>   /*
>* Print (null) with the same width as a pointer so it makes
>* tabular output look nice.
> -- 
> 2.15.1
> 

Re: [PATCH] vsprintf: avoid misleading "(null)" for %px

[Petr Mladek]

Hi,

I add people who actively commented on adding %px modifier,
see the thread starting at
https://lkml.kernel.org/r/1511921105-3647-5-git-send-email...@tobin.cc

Just for reference. It seems to be related to the commit 9f36e2c448007b54
("printk: use %pK for /proc/kallsyms and /proc/modules").


On Sun 2018-02-04 18:45:21, Adam Borowski wrote:
> Like %pK already does, print "" instead.
>
> This confused people -- the convention is that "(null)" means you tried to
> dereference a null pointer as opposed to printing the address.

By other words, this avoids regressions when people convert
%x to %px. Do I get it right, please?

> Signed-off-by: Adam Borowski 
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 77ee6ced11b1..d7a708f82559 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, 
> void *ptr,
>  {
>   const int default_width = 2 * sizeof(void *);
>  
> - if (!ptr && *fmt != 'K') {
> + if (!ptr && *fmt != 'K' && *fmt != 'x') {
>   /*
>* Print (null) with the same width as a pointer so it makes
>* tabular output look nice.
> -- 
> 2.15.1
>