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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1701041320140.3073@hadrien>
Date:   Wed, 4 Jan 2017 13:23:41 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
cc:     Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Kees Cook <keescook@...omium.org>, andrew@...n.ch,
        Jason Cooper <jason@...edaemon.net>,
        rtc-linux@...glegroups.com, a.zummo@...ertech.it,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        gregory.clement@...e-electrons.com,
        Bhumika Goyal <bhumirks@...il.com>,
        sebastian.hesselbarth@...il.com
Subject: Re: [PATCH] rtc: armada38x: add __ro_after_init to
 armada38x_rtc_ops



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