Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471239 > unrolled thread

[PATCH] thermal: fix of_table.cocci warnings

Back to article view | Back to linux.kernel

Contents

  [PATCH] thermal: fix of_table.cocci warnings Julia Lawall <julia.lawall@lip6.fr> - 2016-08-27 21:00 +0200
    constification and cocci / kernel build  test robot ? Joe Perches <joe@perches.com> - 2016-08-27 21:20 +0200
      Re: constification and cocci / kernel build test robot ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 11:50 +0200
      Re: constification and cocci / kernel build test robot ? Joe Perches <joe@perches.com> - 2016-08-28 19:40 +0200
        Re: constification and cocci / kernel build test robot ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 19:50 +0200
      Re: constification and cocci / kernel build test robot ? Kees Cook <keescook@chromium.org> - 2016-08-30 21:00 +0200
        Re: constification and cocci / kernel build test robot ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-30 21:30 +0200
          Re: constification and cocci / kernel build test robot ? Kees Cook <keescook@chromium.org> - 2016-08-31 00:20 +0200
            Re: constification and cocci / kernel build test robot ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-31 07:30 +0200
        Re: constification and cocci / kernel build test robot ? Mark Rutland <mark.rutland@arm.com> - 2016-08-31 12:20 +0200
          Re: constification and cocci / kernel build test robot ? Kees Cook <keescook@chromium.org> - 2016-08-31 16:50 +0200
            Re: constification and cocci / kernel build test robot ? Mark Rutland <mark.rutland@arm.com> - 2016-08-31 18:50 +0200
              Re: constification and cocci / kernel build test robot ? "PaX Team" <pageexec@freemail.hu> - 2016-09-01 17:00 +0200
    Re: [PATCH] thermal: fix of_table.cocci warnings Bin Gao <bin.gao@linux.intel.com> - 2016-08-29 19:00 +0200

#1471239 — [PATCH] thermal: fix of_table.cocci warnings

 Make sure (of/i2c/platform)_device_id tables are NULL terminated
Generated by: scripts/coccinelle/misc/of_table.cocci

CC: Yegnesh S Iyer <yegnesh.s.iyer@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

Please take the patch only if it's a positive warning. Thanks!

 intel_bxt_pmic_thermal.c |    1 +
 1 file changed, 1 insertion(+)

--- a/drivers/thermal/intel_bxt_pmic_thermal.c
+++ b/drivers/thermal/intel_bxt_pmic_thermal.c
@@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
 		.name = "bxt_wcove_thermal",
 		.driver_data = (kernel_ulong_t)&bxtwc_thermal_data,
 	},
+	{},
 };

 static struct platform_driver pmic_thermal_driver = {

[toc] | [next] | [standalone]

#1471243 — constification and cocci / kernel build test robot ?

On Sat, 2016-08-27 at 20:59 +0200, Julia Lawall wrote:
>  Make sure (of/i2c/platform)_device_id tables are NULL terminated
> Generated by: scripts/coccinelle/misc/of_table.cocci

Along the same lines, I submitted a manually generated
patch to add const to some of these structs.

https://lkml.org/lkml/2016/8/26/494

Could cocci and/or the kbuild test robot check for structs
that could or should be const?


> Please take the patch only if it's a positive warning. Thanks!
[]
> +++ b/drivers/thermal/intel_bxt_pmic_thermal.c
> @@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
>                 .name = "bxt_wcove_thermal",
>                 .driver_data = (kernel_ulong_t)&bxtwc_thermal_data,
>         },
> +       {},
>  };
> 
>  static struct platform_driver pmic_thermal_driver = {

[toc] | [prev] | [next] | [standalone]

#1471315 — Re: constification and cocci / kernel build test robot ?

[Multipart message — attachments visible in raw view] — view raw

On Sat, 27 Aug 2016, Joe Perches wrote:

> On Sat, 2016-08-27 at 20:59 +0200, Julia Lawall wrote:
> >  Make sure (of/i2c/platform)_device_id tables are NULL terminated
> > Generated by: scripts/coccinelle/misc/of_table.cocci
>
> Along the same lines, I submitted a manually generated
> patch to add const to some of these structs.
>
> https://lkml.org/lkml/2016/8/26/494
>
> Could cocci and/or the kbuild test robot check for structs
> that could or should be const?

Possibly.  I have sent a lot of patches to constify things, but I used gcc
to verify that the constification was OK.  I could also just send you a
list of the structures that I have constified, or better yet a list of the
structures that are currently always const in the kernel, and you could
perhaps just add them all to checkpatch.  Many of them are not used very
often.  If it would be better to only check for structures that are eg
referenced at least some number of times, or in some number of files, or
in more than one leaf subdirectory, all of that would be possible.

There are also now the "read only after init" annotations that we could be
encouraging people to add.  Kees Cook talked about this at the security
summit last week.  There is thus a need to be sure that things are
annotated as __init that should be as well.

julia


>
>
> > Please take the patch only if it's a positive warning. Thanks!
> []
> > +++ b/drivers/thermal/intel_bxt_pmic_thermal.c
> > @@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
> >                 .name = "bxt_wcove_thermal",
> >                 .driver_data = (kernel_ulong_t)&bxtwc_thermal_data,
> >         },
> > +       {},
> >  };
> >
> >  static struct platform_driver pmic_thermal_driver = {
>

[toc] | [prev] | [next] | [standalone]

#1471411 — Re: constification and cocci / kernel build test robot ?

On Sun, 2016-08-28 at 15:13 +0200, Julia Lawall wrote:
> [Adding Kees, in case it's of interest]

> Below is the list of types of top-level initialized structures and the
> number that are const.  For quicker reading, here are some that are
> sometimes const (numerator), but not always (denominator):

[]

And here is the checkpatch list:

# check for various structs that are normally const (ops, kgdb, device_tree)
		my $const_structs = qr{
				acpi_dock_ops|
				address_space_operations|
				backlight_ops|
				block_device_operations|
				dentry_operations|
				dev_pm_ops|
				dma_map_ops|
				extent_io_ops|
				file_lock_operations|
				file_operations|
				hv_ops|
				ide_dma_ops|
				intel_dvo_dev_ops|
				item_operations|
				iwl_ops|
				kgdb_arch|
				kgdb_io|
				kset_uevent_ops|
				lock_manager_operations|
				microcode_ops|
				mtrr_ops|
				neigh_ops|
				nlmsvc_binding|
				of_device_id|
				pci_raw_ops|
				pipe_buf_operations|
				platform_hibernation_ops|
				platform_suspend_ops|
				proto_ops|
				rpc_pipe_ops|
				seq_operations|
				snd_ac97_build_ops|
				soc_pcmcia_socket_ops|
				stacktrace_ops|
				sysfs_ops|
				tty_operations|
				uart_ops|
				usb_mon_operations|
				wd_ops}x;

While I don't know that Julia's "always const" list is
completely appropriate given possible local naming clashes,
maybe checkpatch can use some separate file with the
structs enumerated like a .checkpatch_const_structs file or
similar to make this more easily editable.

[toc] | [prev] | [next] | [standalone]

#1471420 — Re: constification and cocci / kernel build test robot ?

[Multipart message — attachments visible in raw view] — view raw

On Sun, 28 Aug 2016, Joe Perches wrote:

> On Sun, 2016-08-28 at 15:13 +0200, Julia Lawall wrote:
> > [Adding Kees, in case it's of interest]
>
> > Below is the list of types of top-level initialized structures and the
> > number that are const.  For quicker reading, here are some that are
> > sometimes const (numerator), but not always (denominator):
>
> []
>
> And here is the checkpatch list:
>
> # check for various structs that are normally const (ops, kgdb, device_tree)
> 		my $const_structs = qr{
> 				acpi_dock_ops|
> 				address_space_operations|
> 				backlight_ops|
> 				block_device_operations|
> 				dentry_operations|
> 				dev_pm_ops|
> 				dma_map_ops|
> 				extent_io_ops|
> 				file_lock_operations|
> 				file_operations|
> 				hv_ops|
> 				ide_dma_ops|
> 				intel_dvo_dev_ops|
> 				item_operations|
> 				iwl_ops|
> 				kgdb_arch|
> 				kgdb_io|
> 				kset_uevent_ops|
> 				lock_manager_operations|
> 				microcode_ops|
> 				mtrr_ops|
> 				neigh_ops|
> 				nlmsvc_binding|
> 				of_device_id|
> 				pci_raw_ops|
> 				pipe_buf_operations|
> 				platform_hibernation_ops|
> 				platform_suspend_ops|
> 				proto_ops|
> 				rpc_pipe_ops|
> 				seq_operations|
> 				snd_ac97_build_ops|
> 				soc_pcmcia_socket_ops|
> 				stacktrace_ops|
> 				sysfs_ops|
> 				tty_operations|
> 				uart_ops|
> 				usb_mon_operations|
> 				wd_ops}x;
>
> While I don't know that Julia's "always const" list is
> completely appropriate given possible local naming clashes,
> maybe checkpatch can use some separate file with the
> structs enumerated like a .checkpatch_const_structs file or
> similar to make this more easily editable.

Could be a good idea.

julia

[toc] | [prev] | [next] | [standalone]

#1472780 — Re: constification and cocci / kernel build test robot ?

On Sun, Aug 28, 2016 at 9:13 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> [Adding Kees, in case it's of interest]
>
> Below is the list of types of top-level initialized structures and the
> number that are const.  For quicker reading, here are some that are
> sometimes const (numerator), but not always (denominator):
>
> file_operations: 2221/2233
> attribute_group: 447/919
> irq_chip: 1/518
> net_device_ops: 488/498
> regmap_config: 407/447
> dev_pm_ops: 398/415
> clk_ops: 314/386
> resource: 6/385
> seq_operations: 327/328
> snd_pcm_ops: 9/288
>
> and here are the most used ones that are never const at all:
>
> platform_driver: 2943
> platform_device: 2226
> clk_branch: 1131
> i2c_driver: 786
> pci_driver: 781
> omap_hwmod_ocp_if: 670
> omap_hwmod: 582
> notifier_block: 556
> clk: 473
> clk_rcg2: 384
>
> [...]

The structures that should get the greatest level of attention are
those that contain function pointers. The "constify" gcc plugin from
PaX/Grsecurity does this, but it uses a big hammer: it moves all of
them const even if they receive assignment. To handle this, there is
the concept of an open/close method to gain temporary access to the
structure. For example:

drivers/cdrom/cdrom.c:

int register_cdrom(...) {
        ...
        if (!cdo->generic_packet) {
                pax_open_kernel();
                const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
                pax_close_kernel();
        }

(The "const_cast" here is just a macro to convince gcc it's only to
write to a const value, so really it should maybe be called
"unconst_cast", but whatever...)

This allows all of struct cdrom_device_ops to be const, even if they
need to be updated once during registration.

(This is a stronger version of __ro_after_init, which is for things
that are only written during __init.)

AUIU, the goals of the open/close_kernel idea are:
- always inline
- make sure the CPU cannot be interrupted
- BUG if memory is already writable
- make the memory writable only by the current CPU
- update the value
- restore memory permissions
- allow CPU interruption again

This makes sure there aren't races with other CPUs to write things,
and that it's harder to use for an attack since with the "make
writable" code is always followed by a "make read-only" action (i.e.
not separate functions that could be used as a trivial ROP gadget).

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1472806 — Re: constification and cocci / kernel build test robot ?

On Tue, 30 Aug 2016, Kees Cook wrote:

> On Sun, Aug 28, 2016 at 9:13 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > [Adding Kees, in case it's of interest]
> >
> > Below is the list of types of top-level initialized structures and the
> > number that are const.  For quicker reading, here are some that are
> > sometimes const (numerator), but not always (denominator):
> >
> > file_operations: 2221/2233
> > attribute_group: 447/919
> > irq_chip: 1/518
> > net_device_ops: 488/498
> > regmap_config: 407/447
> > dev_pm_ops: 398/415
> > clk_ops: 314/386
> > resource: 6/385
> > seq_operations: 327/328
> > snd_pcm_ops: 9/288
> >
> > and here are the most used ones that are never const at all:
> >
> > platform_driver: 2943
> > platform_device: 2226
> > clk_branch: 1131
> > i2c_driver: 786
> > pci_driver: 781
> > omap_hwmod_ocp_if: 670
> > omap_hwmod: 582
> > notifier_block: 556
> > clk: 473
> > clk_rcg2: 384
> >
> > [...]
>
> The structures that should get the greatest level of attention are
> those that contain function pointers. The "constify" gcc plugin from
> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
> them const even if they receive assignment. To handle this, there is
> the concept of an open/close method to gain temporary access to the
> structure. For example:
>
> drivers/cdrom/cdrom.c:
>
> int register_cdrom(...) {
>         ...
>         if (!cdo->generic_packet) {
>                 pax_open_kernel();
>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
>                 pax_close_kernel();

Thanks for the clarification.  The above has to be added to the code
manually, or the plugin does it?

julia

>         }
>
> (The "const_cast" here is just a macro to convince gcc it's only to
> write to a const value, so really it should maybe be called
> "unconst_cast", but whatever...)
>
> This allows all of struct cdrom_device_ops to be const, even if they
> need to be updated once during registration.
>
> (This is a stronger version of __ro_after_init, which is for things
> that are only written during __init.)
>
> AUIU, the goals of the open/close_kernel idea are:
> - always inline
> - make sure the CPU cannot be interrupted
> - BUG if memory is already writable
> - make the memory writable only by the current CPU
> - update the value
> - restore memory permissions
> - allow CPU interruption again
>
> This makes sure there aren't races with other CPUs to write things,
> and that it's harder to use for an attack since with the "make
> writable" code is always followed by a "make read-only" action (i.e.
> not separate functions that could be used as a trivial ROP gadget).
>
> -Kees
>
> --
> Kees Cook
> Nexus Security
>

[toc] | [prev] | [next] | [standalone]

#1472910 — Re: constification and cocci / kernel build test robot ?

On Tue, Aug 30, 2016 at 3:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Tue, 30 Aug 2016, Kees Cook wrote:
>
>> On Sun, Aug 28, 2016 at 9:13 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > [Adding Kees, in case it's of interest]
>> >
>> > Below is the list of types of top-level initialized structures and the
>> > number that are const.  For quicker reading, here are some that are
>> > sometimes const (numerator), but not always (denominator):
>> >
>> > file_operations: 2221/2233
>> > attribute_group: 447/919
>> > irq_chip: 1/518
>> > net_device_ops: 488/498
>> > regmap_config: 407/447
>> > dev_pm_ops: 398/415
>> > clk_ops: 314/386
>> > resource: 6/385
>> > seq_operations: 327/328
>> > snd_pcm_ops: 9/288
>> >
>> > and here are the most used ones that are never const at all:
>> >
>> > platform_driver: 2943
>> > platform_device: 2226
>> > clk_branch: 1131
>> > i2c_driver: 786
>> > pci_driver: 781
>> > omap_hwmod_ocp_if: 670
>> > omap_hwmod: 582
>> > notifier_block: 556
>> > clk: 473
>> > clk_rcg2: 384
>> >
>> > [...]
>>
>> The structures that should get the greatest level of attention are
>> those that contain function pointers. The "constify" gcc plugin from
>> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
>> them const even if they receive assignment. To handle this, there is
>> the concept of an open/close method to gain temporary access to the
>> structure. For example:
>>
>> drivers/cdrom/cdrom.c:
>>
>> int register_cdrom(...) {
>>         ...
>>         if (!cdo->generic_packet) {
>>                 pax_open_kernel();
>>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
>>                 pax_close_kernel();
>
> Thanks for the clarification.  The above has to be added to the code
> manually, or the plugin does it?

Currently, the plugin just warns, and a successful build depends on
manually adding the open/close logic. For simple cases, the plugin
could be taught to do this automatically, but some situations are more
complex.

-Kees

>
> julia
>
>>         }
>>
>> (The "const_cast" here is just a macro to convince gcc it's only to
>> write to a const value, so really it should maybe be called
>> "unconst_cast", but whatever...)
>>
>> This allows all of struct cdrom_device_ops to be const, even if they
>> need to be updated once during registration.
>>
>> (This is a stronger version of __ro_after_init, which is for things
>> that are only written during __init.)
>>
>> AUIU, the goals of the open/close_kernel idea are:
>> - always inline
>> - make sure the CPU cannot be interrupted
>> - BUG if memory is already writable
>> - make the memory writable only by the current CPU
>> - update the value
>> - restore memory permissions
>> - allow CPU interruption again
>>
>> This makes sure there aren't races with other CPUs to write things,
>> and that it's harder to use for an attack since with the "make
>> writable" code is always followed by a "make read-only" action (i.e.
>> not separate functions that could be used as a trivial ROP gadget).
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Nexus Security
>>



-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1473038 — Re: constification and cocci / kernel build test robot ?

On Tue, 30 Aug 2016, Kees Cook wrote:

> On Tue, Aug 30, 2016 at 3:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Tue, 30 Aug 2016, Kees Cook wrote:
> >
> >> On Sun, Aug 28, 2016 at 9:13 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> > [Adding Kees, in case it's of interest]
> >> >
> >> > Below is the list of types of top-level initialized structures and the
> >> > number that are const.  For quicker reading, here are some that are
> >> > sometimes const (numerator), but not always (denominator):
> >> >
> >> > file_operations: 2221/2233
> >> > attribute_group: 447/919
> >> > irq_chip: 1/518
> >> > net_device_ops: 488/498
> >> > regmap_config: 407/447
> >> > dev_pm_ops: 398/415
> >> > clk_ops: 314/386
> >> > resource: 6/385
> >> > seq_operations: 327/328
> >> > snd_pcm_ops: 9/288
> >> >
> >> > and here are the most used ones that are never const at all:
> >> >
> >> > platform_driver: 2943
> >> > platform_device: 2226
> >> > clk_branch: 1131
> >> > i2c_driver: 786
> >> > pci_driver: 781
> >> > omap_hwmod_ocp_if: 670
> >> > omap_hwmod: 582
> >> > notifier_block: 556
> >> > clk: 473
> >> > clk_rcg2: 384
> >> >
> >> > [...]
> >>
> >> The structures that should get the greatest level of attention are
> >> those that contain function pointers. The "constify" gcc plugin from
> >> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
> >> them const even if they receive assignment. To handle this, there is
> >> the concept of an open/close method to gain temporary access to the
> >> structure. For example:
> >>
> >> drivers/cdrom/cdrom.c:
> >>
> >> int register_cdrom(...) {
> >>         ...
> >>         if (!cdo->generic_packet) {
> >>                 pax_open_kernel();
> >>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
> >>                 pax_close_kernel();
> >
> > Thanks for the clarification.  The above has to be added to the code
> > manually, or the plugin does it?
>
> Currently, the plugin just warns, and a successful build depends on
> manually adding the open/close logic. For simple cases, the plugin
> could be taught to do this automatically, but some situations are more
> complex.

I guess it would be desirable to avoid this if at all possible. But maybe
it would end up in some library functions, because some drivers would call
them from an init context.

julia

[toc] | [prev] | [next] | [standalone]

#1473247 — Re: constification and cocci / kernel build test robot ?

On Tue, Aug 30, 2016 at 02:50:36PM -0400, Kees Cook wrote:
> The structures that should get the greatest level of attention are
> those that contain function pointers. The "constify" gcc plugin from
> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
> them const even if they receive assignment. To handle this, there is
> the concept of an open/close method to gain temporary access to the
> structure. For example:
> 
> drivers/cdrom/cdrom.c:
> 
> int register_cdrom(...) {
>         ...
>         if (!cdo->generic_packet) {
>                 pax_open_kernel();
>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
>                 pax_close_kernel();
>         }
> 
> (The "const_cast" here is just a macro to convince gcc it's only to
> write to a const value, so really it should maybe be called
> "unconst_cast", but whatever...)

Just to check, are they actually marked as const, or some pseudo-const
like __ro_after_init?

I can imagine a number of issues with casting a real const away (e.g. if
the compiler decides to cache the value in a register and decides since
it is const it need not hazard with a memory clobber).

> This allows all of struct cdrom_device_ops to be const, even if they
> need to be updated once during registration.
> 
> (This is a stronger version of __ro_after_init, which is for things
> that are only written during __init.)
> 
> AUIU, the goals of the open/close_kernel idea are:
> - always inline
> - make sure the CPU cannot be interrupted

I guess s/be interrupted/take an exception/, to cover stuff like debug
breakpoints and such.

That might not always be possible, and might not strictly be a
requirement, if you can guarantee that taking an exception will cause
the mapping to become non-writeable during the handler.

That's one approach I plan to look into for arm64, assuming that we
create a temporarily RW alias in TTBR0.

> - BUG if memory is already writable
> - make the memory writable only by the current CPU
> - update the value
> - restore memory permissions
> - allow CPU interruption again
> 
> This makes sure there aren't races with other CPUs to write things,
> and that it's harder to use for an attack since with the "make
> writable" code is always followed by a "make read-only" action (i.e.
> not separate functions that could be used as a trivial ROP gadget).

FWIW, on that front we may want to look into reworking the fixmap code.
For at least arm, arm64, microblaze, and powerpc, __set_fixmap is a C
function that might offer a trivial ROP gadget for creating a RW
mapping.

Other architectures have that as a static inline in a header, which
ensures that it's folded into callers, making it less trivial to reuse.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1473559 — Re: constification and cocci / kernel build test robot ?

On Wed, Aug 31, 2016 at 6:08 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Aug 30, 2016 at 02:50:36PM -0400, Kees Cook wrote:
>> The structures that should get the greatest level of attention are
>> those that contain function pointers. The "constify" gcc plugin from
>> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
>> them const even if they receive assignment. To handle this, there is
>> the concept of an open/close method to gain temporary access to the
>> structure. For example:
>>
>> drivers/cdrom/cdrom.c:
>>
>> int register_cdrom(...) {
>>         ...
>>         if (!cdo->generic_packet) {
>>                 pax_open_kernel();
>>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
>>                 pax_close_kernel();
>>         }
>>
>> (The "const_cast" here is just a macro to convince gcc it's only to
>> write to a const value, so really it should maybe be called
>> "unconst_cast", but whatever...)
>
> Just to check, are they actually marked as const, or some pseudo-const
> like __ro_after_init?

The plugin marks them actually const, so the const_cast() is needed to
"forget" that marking and treat it as a normal variable. (And PaX Team
noted that this is the name used in C++ already.)

> I can imagine a number of issues with casting a real const away (e.g. if
> the compiler decides to cache the value in a register and decides since
> it is const it need not hazard with a memory clobber).

AFAIK, this hasn't caused problems. PaX Team may be able to speak more
to this...

>
>> This allows all of struct cdrom_device_ops to be const, even if they
>> need to be updated once during registration.
>>
>> (This is a stronger version of __ro_after_init, which is for things
>> that are only written during __init.)
>>
>> AUIU, the goals of the open/close_kernel idea are:
>> - always inline
>> - make sure the CPU cannot be interrupted
>
> I guess s/be interrupted/take an exception/, to cover stuff like debug
> breakpoints and such.

PaX team corrected me: it's not interrupt blocking but rather than it
shouldn't be preempted.

> That might not always be possible, and might not strictly be a
> requirement, if you can guarantee that taking an exception will cause
> the mapping to become non-writeable during the handler.
>
> That's one approach I plan to look into for arm64, assuming that we
> create a temporarily RW alias in TTBR0.
>
>> - BUG if memory is already writable
>> - make the memory writable only by the current CPU
>> - update the value
>> - restore memory permissions
>> - allow CPU interruption again
>>
>> This makes sure there aren't races with other CPUs to write things,
>> and that it's harder to use for an attack since with the "make
>> writable" code is always followed by a "make read-only" action (i.e.
>> not separate functions that could be used as a trivial ROP gadget).
>
> FWIW, on that front we may want to look into reworking the fixmap code.
> For at least arm, arm64, microblaze, and powerpc, __set_fixmap is a C
> function that might offer a trivial ROP gadget for creating a RW
> mapping.
>
> Other architectures have that as a static inline in a header, which
> ensures that it's folded into callers, making it less trivial to reuse.

Even with the inlining, it's just making things slightly harder, since
there is still a memory write happening, so a careful setup before
calling it can still be used as a ROP gadget.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1473693 — Re: constification and cocci / kernel build test robot ?

Hi Kees,

On Wed, Aug 31, 2016 at 10:44:28AM -0400, Kees Cook wrote:
> On Wed, Aug 31, 2016 at 6:08 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Aug 30, 2016 at 02:50:36PM -0400, Kees Cook wrote:
> >> The structures that should get the greatest level of attention are
> >> those that contain function pointers. The "constify" gcc plugin from
> >> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
> >> them const even if they receive assignment. To handle this, there is
> >> the concept of an open/close method to gain temporary access to the
> >> structure. For example:
> >>
> >> drivers/cdrom/cdrom.c:
> >>
> >> int register_cdrom(...) {
> >>         ...
> >>         if (!cdo->generic_packet) {
> >>                 pax_open_kernel();
> >>                 const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
> >>                 pax_close_kernel();
> >>         }
> >>
> >> (The "const_cast" here is just a macro to convince gcc it's only to
> >> write to a const value, so really it should maybe be called
> >> "unconst_cast", but whatever...)
> >
> > Just to check, are they actually marked as const, or some pseudo-const
> > like __ro_after_init?
> 
> The plugin marks them actually const, so the const_cast() is needed to
> "forget" that marking and treat it as a normal variable. (And PaX Team
> noted that this is the name used in C++ already.)

From having a look around, my understanding is that with C++ this is
only valid if the underlying object is not const (i.e. it's only valid
to remove constness from a pointer or reference which had itself added
constness to a non-const object).

I see that GCC is happy to constant-fold function pointers in const
objects it has visibility of; example below. Making the objects
themselves const is bound to lead to fragility (e.g. static inline
functions in a header behaving differently from related functions in
another file).

Have I misunderstood something?

Note: For the below I manually fixed up the symbol resolution in ret_a,
as by default objdump misleadingly reports <intfunc> rather than <a>.

---->8----
[mark@leverpostej:~]% cat const-test.c                                         
int intfunc(int arg) 
{
        return arg;
}

struct foo {
        int (*func)(int arg);
};

struct foo a = {
        .func = intfunc,
};

const struct foo b = {
        .func = intfunc,
};

extern const struct foo c;

int ret_a(int val)
{
        return a.func(val);
}

int ret_b(int val)
{
        return b.func(val);
}

int ret_c(int val)
{
        return c.func(val);
}
[mark@leverpostej:~]% uselinaro 15.08 aarch64-linux-gnu-gcc -O3 -c const-test.c 
[mark@leverpostej:~]% uselinaro 15.08 aarch64-linux-gnu-objdump -d const-test.o 

const-test.o:     file format elf64-littleaarch64


Disassembly of section .text:

0000000000000000 <intfunc>:
   0:   d65f03c0        ret
   4:   d503201f        nop

0000000000000008 <ret_a>:
   8:   90000001        adrp    x1, 0 <a>
   c:   f9400021        ldr     x1, [x1]
  10:   d61f0020        br      x1
  14:   d503201f        nop

0000000000000018 <ret_b>:
  18:   d65f03c0        ret
  1c:   d503201f        nop

0000000000000020 <ret_c>:
  20:   90000001        adrp    x1, 0 <c>
  24:   f9400021        ldr     x1, [x1]
  28:   d61f0020        br      x1
  2c:   d503201f        nop
---->8----

> > I can imagine a number of issues with casting a real const away (e.g. if
> > the compiler decides to cache the value in a register and decides since
> > it is const it need not hazard with a memory clobber).
> 
> AFAIK, this hasn't caused problems. PaX Team may be able to speak more
> to this...

Per the above, and assuming that I haven't missed something, I cannot
see how this can be safe, even if it happens to have worked so far.

Hopefully I'm just being thick, and someone can correct me. :)

[...]

> >> This makes sure there aren't races with other CPUs to write things,
> >> and that it's harder to use for an attack since with the "make
> >> writable" code is always followed by a "make read-only" action (i.e.
> >> not separate functions that could be used as a trivial ROP gadget).
> >
> > FWIW, on that front we may want to look into reworking the fixmap code.
> > For at least arm, arm64, microblaze, and powerpc, __set_fixmap is a C
> > function that might offer a trivial ROP gadget for creating a RW
> > mapping.
> >
> > Other architectures have that as a static inline in a header, which
> > ensures that it's folded into callers, making it less trivial to reuse.
> 
> Even with the inlining, it's just making things slightly harder, since
> there is still a memory write happening, so a careful setup before
> calling it can still be used as a ROP gadget.

Sure. Hence "less trivial". ;)

If it's not likely to be a noticeable improvement, I'm happy to leave
that as-is. I just thought it was worth mentioning.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1474445 — Re: constification and cocci / kernel build test robot ?

On 31 Aug 2016 at 17:41, Mark Rutland wrote:

> > The plugin marks them actually const, so the const_cast() is needed to
> > "forget" that marking and treat it as a normal variable. (And PaX Team
> > noted that this is the name used in C++ already.)
> 
> From having a look around, my understanding is that with C++ this is
> only valid if the underlying object is not const (i.e. it's only valid
> to remove constness from a pointer or reference which had itself added
> constness to a non-const object).
> 
> I see that GCC is happy to constant-fold function pointers in const
> objects it has visibility of; example below. Making the objects
> themselves const is bound to lead to fragility (e.g. static inline
> functions in a header behaving differently from related functions in
> another file).
> 
> Have I misunderstood something?

it's all correct but it's also not what PaX does for exactly this reason.
instead we only constify types where no global variable instances are accessed
directly. for directly accessed global variables we use __read_only and
pax_open/close_kernel calls. ideally, this should be done for all cases and
then const_cast would no longer be needed however that requires some non-trivial
work on the plugin side.

[toc] | [prev] | [next] | [standalone]

#1471988

On Sat, Aug 27, 2016 at 08:59:12PM +0200, Julia Lawall wrote:
>  Make sure (of/i2c/platform)_device_id tables are NULL terminated
> Generated by: scripts/coccinelle/misc/of_table.cocci
> 
> CC: Yegnesh S Iyer <yegnesh.s.iyer@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
> 
> Please take the patch only if it's a positive warning. Thanks!
> 
>  intel_bxt_pmic_thermal.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/drivers/thermal/intel_bxt_pmic_thermal.c
> +++ b/drivers/thermal/intel_bxt_pmic_thermal.c
> @@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
>  		.name = "bxt_wcove_thermal",
>  		.driver_data = (kernel_ulong_t)&bxtwc_thermal_data,
>  	},
> +	{},
>  };
> 
>  static struct platform_driver pmic_thermal_driver = {

Included in v5. Thanks.

-Bin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web