[rtc-linux] [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[rtc-linux] [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Bhumika Goyal]

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 <bhumirks@gmail.com>
---
 drivers/rtc/rtc-armada38x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 9a3f2a6..7883c7f 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -202,7 +202,7 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static struct rtc_class_ops armada38x_rtc_ops = {
+static struct rtc_class_ops armada38x_rtc_ops __ro_after_init = {
 	.read_time = armada38x_rtc_read_time,
 	.set_time = armada38x_rtc_set_time,
 	.read_alarm = armada38x_rtc_read_alarm,
-- 
1.9.1

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Kees Cook]

On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> 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.

Can the pointer that points to the struct rtc_class_ops be made ro_after_init?

-Kees

-- 
Kees Cook
Nexus Security

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

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
> <linux@armlinux.org.uk> wrote:
> > 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.
> 
> 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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

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:
> > On Mon, Jan 2, 2017 at 6:06 AM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > 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.
> > 
> > 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(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 9a3f2a6f512e..a4166ccfce36 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -202,7 +202,7 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static struct rtc_class_ops armada38x_rtc_ops = {
+static const struct rtc_class_ops armada38x_rtc_ops = {
 	.read_time = armada38x_rtc_read_time,
 	.set_time = armada38x_rtc_set_time,
 	.read_alarm = armada38x_rtc_read_alarm,
@@ -210,8 +210,15 @@ static struct rtc_class_ops armada38x_rtc_ops = {
 	.alarm_irq_enable = armada38x_rtc_alarm_irq_enable,
 };
 
+static const struct rtc_class_ops armada38x_rtc_ops_noirq = {
+	.read_time = armada38x_rtc_read_time,
+	.set_time = armada38x_rtc_set_time,
+	.read_alarm = armada38x_rtc_read_alarm,
+};
+
 static __init int armada38x_rtc_probe(struct platform_device *pdev)
 {
+	const struct rtc_class_ops *ops;
 	struct resource *res;
 	struct armada38x_rtc *rtc;
 	int ret;
@@ -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 {
 		/*
 		 * If there is no interrupt available then we can't
 		 * use the alarm
 		 */
-		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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Julia Lawall]

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:
> > 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
> > > <linux@armlinux.org.uk> wrote:
> > > > 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.
> > >
> > > 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

>
> 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(-)
>
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> index 9a3f2a6f512e..a4166ccfce36 100644
> --- a/drivers/rtc/rtc-armada38x.c
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -202,7 +202,7 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>
> -static struct rtc_class_ops armada38x_rtc_ops = {
> +static const struct rtc_class_ops armada38x_rtc_ops = {
>  	.read_time = armada38x_rtc_read_time,
>  	.set_time = armada38x_rtc_set_time,
>  	.read_alarm = armada38x_rtc_read_alarm,
> @@ -210,8 +210,15 @@ static struct rtc_class_ops armada38x_rtc_ops = {
>  	.alarm_irq_enable = armada38x_rtc_alarm_irq_enable,
>  };
>
> +static const struct rtc_class_ops armada38x_rtc_ops_noirq = {
> +	.read_time = armada38x_rtc_read_time,
> +	.set_time = armada38x_rtc_set_time,
> +	.read_alarm = armada38x_rtc_read_alarm,
> +};
> +
>  static __init int armada38x_rtc_probe(struct platform_device *pdev)
>  {
> +	const struct rtc_class_ops *ops;
>  	struct resource *res;
>  	struct armada38x_rtc *rtc;
>  	int ret;
> @@ -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 {
>  		/*
>  		 * If there is no interrupt available then we can't
>  		 * use the alarm
>  		 */
> -		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.
>

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Alexandre Belloni]

On 04/01/2017 at 11:57:00 +0100, Julia Lawall wrote :
> 
> 
> 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:
> > > 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
> > > > <linux@armlinux.org.uk> wrote:
> > > > > 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.
> > > >
> > > > 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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Julia Lawall]

> 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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

On Wed, Jan 04, 2017 at 12:43:32PM +0100, Julia Lawall wrote:
> > 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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Julia Lawall]

On Wed, 4 Jan 2017, Russell King - ARM Linux wrote:

> On Wed, Jan 04, 2017 at 12:43:32PM +0100, Julia Lawall wrote:
> > > 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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Julia Lawall]

On Wed, 4 Jan 2017, Russell King - ARM Linux wrote:

> 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.

OK, thanks for the explanations.

julia

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Kees Cook]

On Wed, Jan 4, 2017 at 5:06 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> 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.

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.