The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
Signed-off-by: Bhumika Goyal <redacted>
---
drivers/rtc/rtc-armada38x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-01-02 14:07:22
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
quoted
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
Can the pointer that points to the struct rtc_class_ops be made ro_after_init?
-Kees
--
Kees Cook
Nexus Security
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-01-03 21:32:24
On Tue, Jan 03, 2017 at 01:18:29PM -0800, Kees Cook wrote:
On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
quoted
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
Can the pointer that points to the struct rtc_class_ops be made ro_after_init?
It's passed into the RTC core code, and probably stored in some dynamically
allocated object, so probably no. It's the same class of problem as every
file_operations pointer in the kernel, or the thousand other operations
structure pointers that a running kernel has.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-01-03 21:54:57
On Tue, Jan 03, 2017 at 09:31:18PM +0000, Russell King - ARM Linux wrote:
On Tue, Jan 03, 2017 at 01:18:29PM -0800, Kees Cook wrote:
quoted
On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
quoted
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
Can the pointer that points to the struct rtc_class_ops be made ro_after_init?
It's passed into the RTC core code, and probably stored in some dynamically
allocated object, so probably no. It's the same class of problem as every
file_operations pointer in the kernel, or the thousand other operations
structure pointers that a running kernel has.
For the elimination of doubt, this is what I meant in my original email.
As you can see, there's nothing to be marked as __ro_after_init anymore.
drivers/rtc/rtc-armada38x.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
@@ -242,19 +249,22 @@ static __init int armada38x_rtc_probe(struct platform_device *pdev)0,pdev->name,rtc)<0){dev_warn(&pdev->dev,"Interrupt not available.\n");rtc->irq=-1;+}+platform_set_drvdata(pdev,rtc);++if(rtc->irq!=-1){+device_init_wakeup(&pdev->dev,1);+ops=&armada38x_rtc_ops;+}else{/**Ifthereisnointerruptavailablethenwecan't*usethealarm*/-armada38x_rtc_ops.set_alarm=NULL;-armada38x_rtc_ops.alarm_irq_enable=NULL;+ops=&armada38x_rtc_ops_noirq;}-platform_set_drvdata(pdev,rtc);-if(rtc->irq!=-1)-device_init_wakeup(&pdev->dev,1);rtc->rtc_dev=devm_rtc_device_register(&pdev->dev,pdev->name,-&armada38x_rtc_ops,THIS_MODULE);+ops,THIS_MODULE);if(IS_ERR(rtc->rtc_dev)){ret=PTR_ERR(rtc->rtc_dev);dev_err(&pdev->dev,"Failed to register RTC device: %d\n",ret);
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Julia Lawall <hidden> Date: 2017-01-04 10:57:57
On Tue, 3 Jan 2017, Russell King - ARM Linux wrote:
On Tue, Jan 03, 2017 at 09:31:18PM +0000, Russell King - ARM Linux wrote:
quoted
On Tue, Jan 03, 2017 at 01:18:29PM -0800, Kees Cook wrote:
quoted
On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
quoted
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
Can the pointer that points to the struct rtc_class_ops be made ro_after_init?
It's passed into the RTC core code, and probably stored in some dynamically
allocated object, so probably no. It's the same class of problem as every
file_operations pointer in the kernel, or the thousand other operations
structure pointers that a running kernel has.
I'm not sure to understand the question and the response. A quick check
with grep suggests that most rtc_class_ops pointers are already const.
There seem to be just some instances in specific drivers that are not.
julia
quoted hunk
For the elimination of doubt, this is what I meant in my original email.
As you can see, there's nothing to be marked as __ro_after_init anymore.
drivers/rtc/rtc-armada38x.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
On 04/01/2017 at 11:57:00 +0100, Julia Lawall wrote :
On Tue, 3 Jan 2017, Russell King - ARM Linux wrote:
quoted
On Tue, Jan 03, 2017 at 09:31:18PM +0000, Russell King - ARM Linux wrote:
quoted
On Tue, Jan 03, 2017 at 01:18:29PM -0800, Kees Cook wrote:
quoted
On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Mon, Dec 26, 2016 at 05:01:02PM +0530, Bhumika Goyal wrote:
quoted
The object armada38x_rtc_ops of type rtc_class_ops structure is not
modified after getting initialized by armada38x_rtc_probe. Apart from
getting referenced in init it is also passed as an argument to the function
devm_rtc_device_register but this argument is of type const struct
rtc_class_ops *. Therefore add __ro_after_init to its declaration.
What I'd prefer here is for the structure to be duplicated, with one
copy having the alarm methods and one which does not. Both can then
be made "const" (so placed into the read-only section at link time)
and the probe function select between the two.
I think that's a cleaner and better solution, even though it's
slightly larger.
I'm not a fan of __ro_after_init being used where other solutions are
possible.
Can the pointer that points to the struct rtc_class_ops be made ro_after_init?
It's passed into the RTC core code, and probably stored in some dynamically
allocated object, so probably no. It's the same class of problem as every
file_operations pointer in the kernel, or the thousand other operations
structure pointers that a running kernel has.
I'm not sure to understand the question and the response. A quick check
with grep suggests that most rtc_class_ops pointers are already const.
There seem to be just some instances in specific drivers that are not.
The question was whether the point to the rtc_class_ops could be made
__ro_after_init. And Russell is right, it is pointed to by the ops
pointer in a struct rtc_device and that struct is dynamically allocated
in rtc_device_register().
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Julia Lawall <hidden> Date: 2017-01-04 11:45:06
The question was whether the point to the rtc_class_ops could be made
__ro_after_init. And Russell is right, it is pointed to by the ops
pointer in a struct rtc_device and that struct is dynamically allocated
in rtc_device_register().
OK, I think it's a terminology issue. You mean the structure that
contains the pointer, and not the pointer itself, which is already const.
thanks,
julia
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-01-04 12:15:56
On Wed, Jan 04, 2017 at 12:43:32PM +0100, Julia Lawall wrote:
quoted
The question was whether the point to the rtc_class_ops could be made
__ro_after_init. And Russell is right, it is pointed to by the ops
pointer in a struct rtc_device and that struct is dynamically allocated
in rtc_device_register().
OK, I think it's a terminology issue. You mean the structure that
contains the pointer, and not the pointer itself, which is already const.
That statement is really ambiguous, and really doesn't help the cause -
we have several structures here which contain pointers and it's far from
clear which you're talking about:
- The armada38x_rtc_ops and armada38x_rtc_ops_noirq structures contain
pointers to functions.
- The dynamically allocated struct rtc_device contains an ops pointer,
which will point at one or other of these two structures.
Now, as soon as we make armada38x_rtc_ops and armada38x_rtc_ops_noirq
const, if the pointer is passed through any function call where the
argument is not also marked const, or is assigned to a pointer that is
not marked const (without an explicit cast), the compiler will complain.
Remember that a const pointer (iow, const void *ptr) is just a hint to
the compiler that "ptr" _may_ point at read-only data, and dereferences
of the pointer are not allowed to write - it's just syntactic checking.
Given that this is stuff we should all know, I'm not quite sure what
people are getting in a tiz over... I'm finding it worrying that I'm
even writing this mail, reviewing this stuff! _Really_ worried that
Kees even brought it up in the first place - I suspect that came from
a misunderstanding of my suggestion which is why I later provided the
suggestion in patch form.
What I suggested, and what my patch does is:
1. It places both the armada38x_rtc_ops and armada38x_rtc_ops_noirq
structures into the .rodata section, which will be protected from
writes by hardware when appropriate kernel options are enabled.
2. The driver does _not_ store a local pointer to this memory at a
static address which could be subsequently modified (*).
3. The only pointer to this memory is during driver initialisation
(which may well reside in a CPU register only) before being passed
to the RTC subsystem.
4. The RTC subsystem dynamically allocates a struct rtc_device
structure (in rtc_device_register()) where it eventually stores
this pointer. This pointer is already marked const. This structure
contains read/write data, and can't be marked read-only, just in the
same way as "struct file" can't be.
The whole __ro_after_init thing is completely irrelevant and a total
distraction at this point - there is nothing that you could add a
__ro_after_init annotation to after my patch in regard of these ops
structures.
* - however, a compiler may decide to store the addresses of these
structures as a literal constant near the function, but with RONX
protection for the .text section, this memory is also read-only, and
so can't be modified.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Julia Lawall <hidden> Date: 2017-01-04 12:23:55
On Wed, 4 Jan 2017, Russell King - ARM Linux wrote:
On Wed, Jan 04, 2017 at 12:43:32PM +0100, Julia Lawall wrote:
quoted
quoted
The question was whether the point to the rtc_class_ops could be made
__ro_after_init. And Russell is right, it is pointed to by the ops
pointer in a struct rtc_device and that struct is dynamically allocated
in rtc_device_register().
OK, I think it's a terminology issue. You mean the structure that
contains the pointer, and not the pointer itself, which is already const.
That statement is really ambiguous, and really doesn't help the cause -
we have several structures here which contain pointers and it's far from
clear which you're talking about:
- The armada38x_rtc_ops and armada38x_rtc_ops_noirq structures contain
pointers to functions.
- The dynamically allocated struct rtc_device contains an ops pointer,
which will point at one or other of these two structures.
Now, as soon as we make armada38x_rtc_ops and armada38x_rtc_ops_noirq
const, if the pointer is passed through any function call where the
argument is not also marked const, or is assigned to a pointer that is
not marked const (without an explicit cast), the compiler will complain.
Remember that a const pointer (iow, const void *ptr) is just a hint to
the compiler that "ptr" _may_ point at read-only data, and dereferences
of the pointer are not allowed to write - it's just syntactic checking.
Given that this is stuff we should all know, I'm not quite sure what
people are getting in a tiz over... I'm finding it worrying that I'm
even writing this mail, reviewing this stuff! _Really_ worried that
Kees even brought it up in the first place - I suspect that came from
a misunderstanding of my suggestion which is why I later provided the
suggestion in patch form.
What I suggested, and what my patch does is:
1. It places both the armada38x_rtc_ops and armada38x_rtc_ops_noirq
structures into the .rodata section, which will be protected from
writes by hardware when appropriate kernel options are enabled.
2. The driver does _not_ store a local pointer to this memory at a
static address which could be subsequently modified (*).
3. The only pointer to this memory is during driver initialisation
(which may well reside in a CPU register only) before being passed
to the RTC subsystem.
4. The RTC subsystem dynamically allocates a struct rtc_device
structure (in rtc_device_register()) where it eventually stores
this pointer. This pointer is already marked const. This structure
contains read/write data, and can't be marked read-only, just in the
same way as "struct file" can't be.
The whole __ro_after_init thing is completely irrelevant and a total
distraction at this point - there is nothing that you could add a
__ro_after_init annotation to after my patch in regard of these ops
structures.
* - however, a compiler may decide to store the addresses of these
structures as a literal constant near the function, but with RONX
protection for the .text section, this memory is also read-only, and
so can't be modified.
Thanks for the explanation. I understood the patch, just not Kees's
question.
Basically, the strategy of the patch is that one may consider it
preferable to duplicate the structure for the different alternatives,
rather than use __ro_after_init. Perhaps if the structure were larger,
then __ro_after_init would be a better choice?
thanks,
julia
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-01-04 13:07:40
On Wed, Jan 04, 2017 at 01:23:41PM +0100, Julia Lawall wrote:
Basically, the strategy of the patch is that one may consider it
preferable to duplicate the structure for the different alternatives,
rather than use __ro_after_init. Perhaps if the structure were larger,
then __ro_after_init would be a better choice?
It depends on not just the size, but how many members need to be
modified, and obviously whether there are likely to be more than one
user of the structure as well.
So I'd say __ro_after_init rarely makes sense for an operations
structure - the only case I can see is:
- a large structure
- only a small number of elements need to be modified
- it is only single-use
which is probably quite rare - this one falls into two out of those
three.
There's another consideration (imho) too - we may wish, at a later
date, to make .text and .rodata both read-only from the start of the
kernel to harden the kernel against possibly init-time exploitation.
(Think about a buggy built-in driver with emulated hardware - much the
same problem that Kees is trying to address in one of his recent patch
sets but with hotplugged hardware while a screen-saver is active.)
Having function pointers in .rodata rather than the ro-after-init
section would provide better protection.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Julia Lawall <hidden> Date: 2017-01-04 13:41:43
On Wed, 4 Jan 2017, Russell King - ARM Linux wrote:
On Wed, Jan 04, 2017 at 01:23:41PM +0100, Julia Lawall wrote:
quoted
Basically, the strategy of the patch is that one may consider it
preferable to duplicate the structure for the different alternatives,
rather than use __ro_after_init. Perhaps if the structure were larger,
then __ro_after_init would be a better choice?
It depends on not just the size, but how many members need to be
modified, and obviously whether there are likely to be more than one
user of the structure as well.
So I'd say __ro_after_init rarely makes sense for an operations
structure - the only case I can see is:
- a large structure
- only a small number of elements need to be modified
- it is only single-use
which is probably quite rare - this one falls into two out of those
three.
There's another consideration (imho) too - we may wish, at a later
date, to make .text and .rodata both read-only from the start of the
kernel to harden the kernel against possibly init-time exploitation.
(Think about a buggy built-in driver with emulated hardware - much the
same problem that Kees is trying to address in one of his recent patch
sets but with hotplugged hardware while a screen-saver is active.)
Having function pointers in .rodata rather than the ro-after-init
section would provide better protection.
On Wed, Jan 4, 2017 at 5:06 AM, Russell King - ARM Linux
[off-list ref] wrote:
On Wed, Jan 04, 2017 at 01:23:41PM +0100, Julia Lawall wrote:
quoted
Basically, the strategy of the patch is that one may consider it
preferable to duplicate the structure for the different alternatives,
rather than use __ro_after_init. Perhaps if the structure were larger,
then __ro_after_init would be a better choice?
It depends on not just the size, but how many members need to be
modified, and obviously whether there are likely to be more than one
user of the structure as well.
So I'd say __ro_after_init rarely makes sense for an operations
structure - the only case I can see is:
- a large structure
- only a small number of elements need to be modified
- it is only single-use
which is probably quite rare - this one falls into two out of those
three.
There's another consideration (imho) too - we may wish, at a later
date, to make .text and .rodata both read-only from the start of the
kernel to harden the kernel against possibly init-time exploitation.
(Think about a buggy built-in driver with emulated hardware - much the
same problem that Kees is trying to address in one of his recent patch
sets but with hotplugged hardware while a screen-saver is active.)
Having function pointers in .rodata rather than the ro-after-init
section would provide better protection.
Agreed: I'd much prefer things just be const. :) As to my confusing
question, I hadn't looked at how where the pointers to the structure
was being stored, so I was just asking if it, too, could be const,
which it can't, and that's fine here.
-Kees
--
Kees Cook
Nexus Security