Re: [PATCH] format-security: move static strings to const

[Kees Cook]

On Thu, Apr 6, 2017 at 1:48 AM, Jani Nikula  wrote:
> On Thu, 06 Apr 2017, Kees Cook  wrote:
>> While examining output from trial builds with -Wformat-security enabled,
>> many strings were found that should be defined as "const", or as a char
>> array instead of char pointer. This makes some static analysis easier,
>> by producing fewer false positives.
>>
>> As these are all trivial changes, it seemed best to put them all in
>> a single patch rather than chopping them up per maintainer.
>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c 
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index f6d4d9700734..1ff9d5912b83 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2331,7 +2331,7 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>>  int __init drm_fb_helper_modinit(void)
>>  {
>>  #if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
>> - const char *name = "fbcon";
>> + const char name[] = "fbcon";
>
> I'd always write the former out of habit. Why should I start using the
> latter? What makes it better?

For me, it's mainly two reasons: sizeof() and -Wformat-security behavior.

The compiler treats "sizeof" differently. E.g. "sizeof(var)" shows the
allocation size for the array, and pointer size for the char pointer.
When doing things like snprintf(buf, sizeof(buf), ...) will do the
right thing, etc. (This is a poor example for a _const_ string, but
the point is that some calculations still work better with the array
over the pointer.)

The other situation (which is why I noted this to change them) is that
gcc's handling of them is different when faced with -Wformat-security
since it doesn't like to believe that const char pointers are actually
const for the purposes of being a format string.

> What keeps the kernel from accumulating tons more of the former?

Right now, nothing. The good news is that they're relatively rare, and
I notice them when they're added (since I have a -Wformat-security
tree). We could add a warning to checkpatch for suggesting const char
var[] over const char *var, perhaps?

> Here's an interesting comparison of the generated code. I'm a bit
> surprised by what gcc does, I would have expected no difference, like
> clang. https://godbolt.org/g/OdqUvN

Here's your example with sizeof() added, if you're curious...
https://godbolt.org/g/U1zIZK

> The other changes adding const in this patch are, of course, good.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] format-security: move static strings to const

[Jani Nikula]

On Thu, 06 Apr 2017, Kees Cook  wrote:
> While examining output from trial builds with -Wformat-security enabled,
> many strings were found that should be defined as "const", or as a char
> array instead of char pointer. This makes some static analysis easier,
> by producing fewer false positives.
>
> As these are all trivial changes, it seemed best to put them all in
> a single patch rather than chopping them up per maintainer.

> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6d4d9700734..1ff9d5912b83 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2331,7 +2331,7 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>  int __init drm_fb_helper_modinit(void)
>  {
>  #if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
> - const char *name = "fbcon";
> + const char name[] = "fbcon";

I'd always write the former out of habit. Why should I start using the
latter? What makes it better?

What keeps the kernel from accumulating tons more of the former?

Here's an interesting comparison of the generated code. I'm a bit
surprised by what gcc does, I would have expected no difference, like
clang. https://godbolt.org/g/OdqUvN

The other changes adding const in this patch are, of course, good.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] format-security: move static strings to const

[Daniel Vetter]

On Wed, Apr 05, 2017 at 02:47:11PM -0700, Kees Cook wrote:
> While examining output from trial builds with -Wformat-security enabled,
> many strings were found that should be defined as "const", or as a char
> array instead of char pointer. This makes some static analysis easier,
> by producing fewer false positives.
> 
> As these are all trivial changes, it seemed best to put them all in
> a single patch rather than chopping them up per maintainer.
> 
> Signed-off-by: Kees Cook 

Ack on the drm part.
-Daniel

> ---
>  arch/arm/mach-omap2/board-n8x0.c  |  2 +-
>  arch/mips/dec/prom/init.c |  6 +++---
>  arch/mips/kernel/traps.c  |  4 ++--
>  drivers/char/dsp56k.c |  2 +-
>  drivers/cpufreq/powernow-k8.c |  3 ++-
>  drivers/gpu/drm/drm_fb_helper.c   |  2 +-
>  drivers/net/ethernet/amd/atarilance.c |  4 ++--
>  drivers/net/ethernet/amd/declance.c   |  2 +-
>  drivers/net/ethernet/amd/sun3lance.c  |  3 ++-
>  drivers/net/ethernet/cirrus/mac89x0.c |  2 +-
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h |  2 +-
>  drivers/net/ethernet/natsemi/sonic.h  |  2 +-
>  drivers/net/ethernet/toshiba/tc35815.c|  2 +-
>  drivers/net/fddi/defxx.c  |  2 +-
>  drivers/net/hippi/rrunner.c   |  3 ++-
>  drivers/staging/most/mostcore/core.c  |  2 +-
>  drivers/tty/n_hdlc.c  | 10 +-
>  drivers/tty/serial/st-asc.c   |  2 +-
>  net/decnet/af_decnet.c|  3 ++-
>  19 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-n8x0.c 
> b/arch/arm/mach-omap2/board-n8x0.c
> index 6b6fda65fb3b..91272db09fa3 100644
> --- a/arch/arm/mach-omap2/board-n8x0.c
> +++ b/arch/arm/mach-omap2/board-n8x0.c
> @@ -117,7 +117,7 @@ static struct musb_hdrc_platform_data tusb_data = {
>  static void __init n8x0_usb_init(void)
>  {
>   int ret = 0;
> - static char announce[] __initdata = KERN_INFO "TUSB 6010\n";
> + static const char announce[] __initconst = KERN_INFO "TUSB 6010\n";
>  
>   /* PM companion chip power control pin */
>   ret = gpio_request_one(TUSB6010_GPIO_ENABLE, GPIOF_OUT_INIT_LOW,
> diff --git a/arch/mips/dec/prom/init.c b/arch/mips/dec/prom/init.c
> index 4e1761e0a09a..d88eb7a6662b 100644
> --- a/arch/mips/dec/prom/init.c
> +++ b/arch/mips/dec/prom/init.c
> @@ -88,7 +88,7 @@ void __init which_prom(s32 magic, s32 *prom_vec)
>  void __init prom_init(void)
>  {
>   extern void dec_machine_halt(void);
> - static char cpu_msg[] __initdata =
> + static const char cpu_msg[] __initconst =
>   "Sorry, this kernel is compiled for a wrong CPU type!\n";
>   s32 argc = fw_arg0;
>   s32 *argv = (void *)fw_arg1;
> @@ -111,7 +111,7 @@ void __init prom_init(void)
>  #if defined(CONFIG_CPU_R3000)
>   if ((current_cpu_type() == CPU_R4000SC) ||
>   (current_cpu_type() == CPU_R4400SC)) {
> - static char r4k_msg[] __initdata =
> + static const char r4k_msg[] __initconst =
>   "Please recompile with \"CONFIG_CPU_R4x00 = y\".\n";
>   printk(cpu_msg);
>   printk(r4k_msg);
> @@ -122,7 +122,7 @@ void __init prom_init(void)
>  #if defined(CONFIG_CPU_R4X00)
>   if ((current_cpu_type() == CPU_R3000) ||
>   (current_cpu_type() == CPU_R3000A)) {
> - static char r3k_msg[] __initdata =
> + static const char r3k_msg[] __initconst =
>   "Please recompile with \"CONFIG_CPU_R3000 = y\".\n";
>   printk(cpu_msg);
>   printk(r3k_msg);
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index c7d17cfb32f6..2c717db50380 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -2256,8 +2256,8 @@ void set_handler(unsigned long offset, void *addr, 
> unsigned long size)
>   local_flush_icache_range(ebase + offset, ebase + offset + size);
>  }
>  
> -static char panic_null_cerr[] =
> - "Trying to set NULL cache error exception handler";
> +static const char panic_null_cerr[] =
> + "Trying to set NULL cache error exception handler\n";
>  
>  /*
>   * Install uncached CPU exception handler.
> diff --git a/drivers/char/dsp56k.c b/drivers/char/dsp56k.c
> index 50aa9ba91f25..0d7b577e0ff0 100644
> --- a/drivers/char/dsp56k.c
> +++ b/drivers/char/dsp56k.c
> @@ -489,7 +489,7 @@ static const struct file_operations dsp56k_fops = {
>  
>  /** Init and module functions **/
>  
> -static char banner[] __initdata = KERN_INFO "DSP56k driver installed\n";
> +static const char banner[] __initconst = KERN_INFO "DSP56k driver 
> installed\n";
>  
>  static int __init dsp56k_init_driver(void)
>  {
> diff --git