Re: [PATCH] mm: fix RODATA_TEST failure "rodata_test: test data was not read only"

[Kees Cook <keescook@chromium.org>]

On Mon, Oct 2, 2017 at 1:08 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Mon, Oct 02, 2017 at 12:29:45PM -0700, Kees Cook wrote:
>> On Mon, Sep 25, 2017 at 12:41 PM, Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>> > On Mon, Sep 25, 2017 at 04:01:55PM +0000, David Laight wrote:
>> >> From: Segher Boessenkool
>> >> > The compiler puts this item in .sdata, for 32-bit.  There is no .srodata,
>> >> > so if it wants to use a small data section, it must use .sdata .
>> >> >
>> >> > Non-external, non-referenced symbols are not put in .sdata, that is the
>> >> > difference you see with the "static".
>> >> >
>> >> > I don't think there is a bug here.  If you think there is, please open
>> >> > a GCC bug.
>> >>
>> >> The .sxxx sections are for 'small' data that can be accessed (typically)
>> >> using small offsets from a global register.
>> >> This means that all sections must be adjacent in the image.
>> >> So you can't really have readonly small data.
>> >>
>> >> My guess is that the linker script is putting .srodata in with .sdata.
>> >
>> > .srodata does not *exist* (in the ABI).
>>
>> So, I still think this is a bug. The variable is marked const: this is
>> not a _suggestion_. :) If the compiler produces output where the
>> variable is writable, that's a bug.
>
> C11 6.7.3/6: "If an attempt is made to modify an object defined with a
> const-qualified type through use of an lvalue with non-const-qualified
> type, the behavior is undefined."
>
> And that is all that "const" means.
>
> The compiler is free to put this var in *no* data section, or to copy
> it to the stack before using it, or anything else it thinks is a good
> idea.

The kernel depends on const things being read-only. I realize C11 says
this is "undefined", but from a kernel security perspective, const
means read-only, and this is true on other architectures. Now,
strictly speaking, the compiler is just responsible for doing section
assignment for a variable, and the linker then lays things out, but
the result carries the requested memory protections (i.e. read-only,
executable, etc). If "const" is just a hint, then what is the
canonical way to have gcc put a variable into a section that the
linker will always request be kept read-only?

> If you think it would be a good idea for the compiler to change its
> behaviour here, please file a PR (or send a patch).  Please bring
> arguments why we would want to change this.

Sure:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82411

>> I can't tell if this bug is related:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=9571
>
> I don't think so: the only remaining bug there is that a copy of the
> constant is put in .rodata.cst8 (although there is a copy in .sdata2
> already).

Okay, thanks for checking.

-Kees

-- 
Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>