Re: [PATCH] Arm: avoid .init.data to be marked as executable

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 14/06/2021 14:17, Jan Beulich wrote:
> On 14.06.2021 12:32, Julien Grall wrote:
> > On 14/06/2021 12:02, Jan Beulich wrote:
> > > On 14.06.2021 11:41, Julien Grall wrote:
> > > > On 11/06/2021 11:39, Jan Beulich wrote:
> > > > > This confuses disassemblers, at the very least. Move
> > > > > .altinstr_replacement to .init.text,
> > > > The alternative code was borrowed from Linux. The code has now changed
> > > > to cater very large kernel. They used to keep the .altinstr_replacement
> > > > and altinstructions close to each other (albeit they were both in
> > > > .init.text).
> > > >
> > > > I am not entirely why, but I am a bit worry to separate them. What sort
> > > > of test did you do?
> > > Well, just build tests, on the assumption that relocation overflows
> > > would be reported by the linker if the sections ended up too far
> > > apart.
> > Hmmm, fair point. They should also not be further than the original
> > instruction. So there ought to be fine.
> >
> > > > > dropping the redundant ALIGN().
> > > > >
> > > > > Also, to have .altinstr_replacement have consistent attributes in the
> > > > > object files, add "x" to the one instance where it was missing. >
> > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > ---
> > > > > I'm uncertain whether having .altinstr_replacement inside or outside the
> > > > > [_sinittext,_einittext) region is better; I simply followed what we have
> > > > > on the x86 side right now.
> > > > This means the altinstructions will be marked executable in the
> > > > page-table. They technically should not be executable, so I would move
> > > > them outside _einittext and make sure the section is aligned to a PAGE_SIZE.
> > > Hmm, are you saying you bother getting attributes right for .init.*
> > > in the page tables? I ask because we don't on x86, and because it
> > > would seem wasteful to me to pad to PAGE_SIZE just for this. But
> > > you're the maintainer, i.e. I'm merely double checking ...
> > So this is a defense in depth. Your assumption is .init.text is going to
> > disappear after boot. However, if there is a bug that would leave
> > .init.text present then this may add more attack surface. So I think it
> > is a good practice to keep the permission correct.
> >
> > However... looking the alternative code again, there is another reason
> > to move this change out of the range _sinitext - _einittext. The
> > function branch_insn_requires_update() will forbid branch target in
> > another alternative instructions.
> >
> > This is first checking that the target is part of an active text. With
> > this change, this will return true because alternative instruction
> > replacement will be between _sinittext and _einittext.
> >
> > So .altinstructions_replacement must outside of the region [_stinittext,
> > _einittext[.
> I see. But I'm not sure about the defense-in-depth aspect: By putting
> it outside [_sinittext,_einittext) it'll get mapped r/w, while I think
> you were implying that it would become r/o. Not even .init.rodata gets
> mapped r/o.
Yes it is no r/o and that should be fixed at some point. However, I feel 
that r/w is better than allowing execution because some the instructions 
can lead to a DoS if executed on platform not supporting them.
But that's a matter of opinion and I think this confused the messaging here.

> As a result I'm not convinced yet that you really want me to make the
> change.
I wrote "must", so I am not sure what else I could say to convince you 
that I really want to make this change...
To re-iterate, this code will break runtime check in the alternative 
patching code. So the .altinstruction_replacement **should** be placed 
after _einittext.
Cheers,

--
Julien Grall