| Message ID | 20170103215421.GN14217@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. >
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().
> 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
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.
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
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.
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
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
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);