[v2,3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

  Since we have added an is_interp flag to arch_parse_elf_property() we can
drop the has_interp flag from arch_elf_adjust_prot(), the only user was
the arm64 code which no longer needs it and any future users will be able
to use arch_parse_elf_properties() to determine if an interpreter is in
use.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/process.c | 2 +-
 fs/binfmt_elf.c             | 2 +-
 include/linux/elf.h         | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)
  

On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> Since we have added an is_interp flag to arch_parse_elf_property() we can
> drop the has_interp flag from arch_elf_adjust_prot(), the only user was
> the arm64 code which no longer needs it and any future users will be able
> to use arch_parse_elf_properties() to determine if an interpreter is in
> use.

So far so good, but can we also drop the has_interp argument from
arch_parse_elf_properties()?

Cross-check with Yu-Cheng Yu's series, but I don't see this being used
any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally
to each other now, I don't think we should need a has_interp argument to
pass knowledge between the interpreter and executable handling phases
here.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 2 +-
>  fs/binfmt_elf.c             | 2 +-
>  include/linux/elf.h         | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index f7fff4a4c99f..e51c4aa7e048 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -742,7 +742,7 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void)
>  
>  #ifdef CONFIG_BINFMT_ELF
>  int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> -			 bool has_interp, bool is_interp)
> +			 bool is_interp)
>  {
>  	if (prot & PROT_EXEC) {
>  		if (state->flags & ARM64_ELF_INTERP_BTI && is_interp)
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 253ca9969345..1aba4e50e651 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -580,7 +580,7 @@ static inline int make_prot(u32 p_flags, struct arch_elf_state *arch_state,
>  	if (p_flags & PF_X)
>  		prot |= PROT_EXEC;
>  
> -	return arch_elf_adjust_prot(prot, arch_state, has_interp, is_interp);
> +	return arch_elf_adjust_prot(prot, arch_state, is_interp);
>  }
>  
>  /* This is much more generalized than the library routine read function,
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index 1c45ecf29147..d8392531899d 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -101,11 +101,11 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
>  
>  #ifdef CONFIG_ARCH_HAVE_ELF_PROT
>  int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> -			 bool has_interp, bool is_interp);
> +			 bool is_interp);
>  #else
>  static inline int arch_elf_adjust_prot(int prot,
>  				       const struct arch_elf_state *state,
> -				       bool has_interp, bool is_interp)
> +				       bool is_interp)

[...]

Otherwise, looks reasonable.

Cheers
---Dave
  

On 6/9/2021 8:17 AM, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>> Since we have added an is_interp flag to arch_parse_elf_property() we can
>> drop the has_interp flag from arch_elf_adjust_prot(), the only user was
>> the arm64 code which no longer needs it and any future users will be able
>> to use arch_parse_elf_properties() to determine if an interpreter is in
>> use.
> 
> So far so good, but can we also drop the has_interp argument from
> arch_parse_elf_properties()?
> 
> Cross-check with Yu-Cheng Yu's series, but I don't see this being used
> any more (except for passthrough in binfmt_elf.c).
> 
> Since we are treating the interpreter and main executable orthogonally
> to each other now, I don't think we should need a has_interp argument to
> pass knowledge between the interpreter and executable handling phases
> here.
> 

For CET, arch_parse_elf_property() needs to know has_interp and 
is_interp.  Like the following, on top of your patches:

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 607b782afe2c..9e6f142b5cef 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -837,8 +837,15 @@ unsigned long KSTK_ESP(struct task_struct *task)
  }

  int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
-			    bool compat, struct arch_elf_state *state)
+			    bool compat, bool has_interp, bool is_interp,
+			    struct arch_elf_state *state)
  {
+	/*
+	 * Parse static-linked executable or the loader.
+	 */
+	if (has_interp != is_interp)
+		return 0;
+
  	if (type != GNU_PROPERTY_X86_FEATURE_1_AND)
  		return 0;

Thanks,
Yu-cheng
  

On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
> On 6/9/2021 8:17 AM, Dave Martin wrote:
> >On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> >>Since we have added an is_interp flag to arch_parse_elf_property() we can
> >>drop the has_interp flag from arch_elf_adjust_prot(), the only user was
> >>the arm64 code which no longer needs it and any future users will be able
> >>to use arch_parse_elf_properties() to determine if an interpreter is in
> >>use.
> >
> >So far so good, but can we also drop the has_interp argument from
> >arch_parse_elf_properties()?
> >
> >Cross-check with Yu-Cheng Yu's series, but I don't see this being used
> >any more (except for passthrough in binfmt_elf.c).
> >
> >Since we are treating the interpreter and main executable orthogonally
> >to each other now, I don't think we should need a has_interp argument to
> >pass knowledge between the interpreter and executable handling phases
> >here.
> >
> 
> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
> Like the following, on top of your patches:
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 607b782afe2c..9e6f142b5cef 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -837,8 +837,15 @@ unsigned long KSTK_ESP(struct task_struct *task)
>  }
> 
>  int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
> -			    bool compat, struct arch_elf_state *state)
> +			    bool compat, bool has_interp, bool is_interp,
> +			    struct arch_elf_state *state)
>  {
> +	/*
> +	 * Parse static-linked executable or the loader.
> +	 */
> +	if (has_interp != is_interp)
> +		return 0;
> +

[...]

Ah, sorry, I did attempt to check this with your series, but I didn't
attempt to build it.  I must have missed this somehow.

But: does x86 actually need to do this?

For arm64, we've discovered that it is better to treat the ELF
interpreter and main executable independently when applying the ELF
properties.

So, can x86 actually port away from this?  arch_parse_elf_properties()
and arch_adjust_elf_prot() would still know whether the interpreter is
being considered or not, via the is_interp argument to both functions.
This allows interpreter and main executable info to be stashed
independently in the arch_elf_state.

If x86 really needs to carry on following the existing model then that's
fine, but we should try to keep x86 and arm64 aligned if at all possible.

Cheers
---Dave
  

On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:

> > Since we have added an is_interp flag to arch_parse_elf_property() we can
> > drop the has_interp flag from arch_elf_adjust_prot(), the only user was
> > the arm64 code which no longer needs it and any future users will be able
> > to use arch_parse_elf_properties() to determine if an interpreter is in
> > use.

> So far so good, but can we also drop the has_interp argument from
> arch_parse_elf_properties()?

> Cross-check with Yu-Cheng Yu's series, but I don't see this being used
> any more (except for passthrough in binfmt_elf.c).

> Since we are treating the interpreter and main executable orthogonally
> to each other now, I don't think we should need a has_interp argument to
> pass knowledge between the interpreter and executable handling phases
> here.

My thinking was that it might be useful for handling of some
future property in the architecture code to know if there is an
interpreter, providing the information at parse time would let it
set up whatever is needed.  We've been doing this with the arm64
BTI handling and while we're moving away from doing that I could
imagine that there may be some other case where it makes sense,
and it sounds like CET is one.
  

On Thu, Jun 10, 2021 at 02:34:03PM +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> > On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> 
> > > Since we have added an is_interp flag to arch_parse_elf_property() we can
> > > drop the has_interp flag from arch_elf_adjust_prot(), the only user was
> > > the arm64 code which no longer needs it and any future users will be able
> > > to use arch_parse_elf_properties() to determine if an interpreter is in
> > > use.
> 
> > So far so good, but can we also drop the has_interp argument from
> > arch_parse_elf_properties()?
> 
> > Cross-check with Yu-Cheng Yu's series, but I don't see this being used
> > any more (except for passthrough in binfmt_elf.c).
> 
> > Since we are treating the interpreter and main executable orthogonally
> > to each other now, I don't think we should need a has_interp argument to
> > pass knowledge between the interpreter and executable handling phases
> > here.
> 
> My thinking was that it might be useful for handling of some
> future property in the architecture code to know if there is an
> interpreter, providing the information at parse time would let it
> set up whatever is needed.  We've been doing this with the arm64
> BTI handling and while we're moving away from doing that I could
> imagine that there may be some other case where it makes sense,
> and it sounds like CET is one.

It might be useful, but if the use cases are purely hypothetical then it
would still seem preferable to remove it so that it doesn't just hang
around forever as cruft.

I guess we need to wait and see whether x86 really needs this, or just
uses it as a side-effect of following the arm64 code initially.

If it is quicker to stabilise this series heeping the has_interps in,
then I guess we have the option to do that and remove them later on once
we're satisfied they're unlikely to be useful (or not).

Cheers
---Dave
  

On 6/10/2021 2:58 AM, Dave Martin wrote:
> On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
>> On 6/9/2021 8:17 AM, Dave Martin wrote:
>>> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>>>> Since we have added an is_interp flag to arch_parse_elf_property() we can
>>>> drop the has_interp flag from arch_elf_adjust_prot(), the only user was
>>>> the arm64 code which no longer needs it and any future users will be able
>>>> to use arch_parse_elf_properties() to determine if an interpreter is in
>>>> use.
>>>
>>> So far so good, but can we also drop the has_interp argument from
>>> arch_parse_elf_properties()?
>>>
>>> Cross-check with Yu-Cheng Yu's series, but I don't see this being used
>>> any more (except for passthrough in binfmt_elf.c).
>>>
>>> Since we are treating the interpreter and main executable orthogonally
>>> to each other now, I don't think we should need a has_interp argument to
>>> pass knowledge between the interpreter and executable handling phases
>>> here.
>>>
>>
>> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
>> Like the following, on top of your patches:
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 607b782afe2c..9e6f142b5cef 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -837,8 +837,15 @@ unsigned long KSTK_ESP(struct task_struct *task)
>>   }
>>
>>   int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
>> -			    bool compat, struct arch_elf_state *state)
>> +			    bool compat, bool has_interp, bool is_interp,
>> +			    struct arch_elf_state *state)
>>   {
>> +	/*
>> +	 * Parse static-linked executable or the loader.
>> +	 */
>> +	if (has_interp != is_interp)
>> +		return 0;
>> +
> 
> [...]
> 
> Ah, sorry, I did attempt to check this with your series, but I didn't
> attempt to build it.  I must have missed this somehow.
> 
> But: does x86 actually need to do this?
> 
> For arm64, we've discovered that it is better to treat the ELF
> interpreter and main executable independently when applying the ELF
> properties.
> 
> So, can x86 actually port away from this?  arch_parse_elf_properties()
> and arch_adjust_elf_prot() would still know whether the interpreter is
> being considered or not, via the is_interp argument to both functions.
> This allows interpreter and main executable info to be stashed
> independently in the arch_elf_state.
> 
> If x86 really needs to carry on following the existing model then that's
> fine, but we should try to keep x86 and arm64 aligned if at all possible.
>

Yes, for CET's purpose, that should be fine.

Thanks,
Yu-cheng