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

["Yu, Yu-cheng" <yu-cheng.yu@intel.com>]

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