[v1] arm64: module: PLT allowed even !RANDOM_BASE

[PATCH] arm64: module: PLT allowed even !RANDOM_BASE

Posted by Maria Yu 2 years, 3 months ago

Module PLT feature can be enabled even when RANDOM_BASE is disabled.
Break BLT entry counts of relocation types will make module plt entry
allocation fail and finally exec format error for even correct and plt
allocation available modules.

Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
---
 arch/arm64/kernel/module-plts.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index bd69a4e7cd60..21a67d52d7a0 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num,
 		switch (ELF64_R_TYPE(rela[i].r_info)) {
 		case R_AARCH64_JUMP26:
 		case R_AARCH64_CALL26:
-			if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
-				break;
-
 			/*
 			 * We only have to consider branch targets that resolve
 			 * to symbols that are defined in a different section.

base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
-- 
2.17.1

Re: [PATCH] arm64: module: PLT allowed even !RANDOM_BASE

Posted by Arnd Bergmann 2 years, 3 months ago

On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote:
> Module PLT feature can be enabled even when RANDOM_BASE is disabled.
> Break BLT entry counts of relocation types will make module plt entry
> allocation fail and finally exec format error for even correct and plt
> allocation available modules.
>
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>

Adding Ard Biesheuvel to Cc, as he added the check in commit
a257e02579e42 ("arm64/kernel: don't ban ADRP to work around
Cortex-A53 erratum #843419")

>  arch/arm64/kernel/module-plts.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/module-plts.c 
> b/arch/arm64/kernel/module-plts.c
> index bd69a4e7cd60..21a67d52d7a0 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, 
> Elf64_Rela *rela, int num,
>  		switch (ELF64_R_TYPE(rela[i].r_info)) {
>  		case R_AARCH64_JUMP26:
>  		case R_AARCH64_CALL26:
> -			if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> -				break;
> -
>  			/*
>  			 * We only have to consider branch targets that resolve
>  			 * to symbols that are defined in a different section.

I see there are two such checks (in partition_branch_plt_relas()
and in count_plts()), can you explain in more detail how you
concluded that one of them is correct but the other one is not?

     Arnd

Re: [PATCH] arm64: module: PLT allowed even !RANDOM_BASE

Posted by Mark Rutland 2 years, 3 months ago

On Mon, Oct 23, 2023 at 10:08:33AM +0200, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote:
> > Module PLT feature can be enabled even when RANDOM_BASE is disabled.
> > Break BLT entry counts of relocation types will make module plt entry
> > allocation fail and finally exec format error for even correct and plt
> > allocation available modules.

Has an actual problem been seen in practice, or was this found by looking at
the code?

> >
> > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> 
> Adding Ard Biesheuvel to Cc, as he added the check in commit
> a257e02579e42 ("arm64/kernel: don't ban ADRP to work around
> Cortex-A53 erratum #843419")

I think that the actual mistake is in commit:

  3e35d303ab7d22c4 ("arm64: module: rework module VA range selection")

Prior to that commit, when CONFIG_RANDOMIZE_BASE=n all modules and code had to
be within 128M of each other, and so there were no PLTs necessary for B/BL.
After that commit we can have a 2G module range regardless of
CONFIG_RANDOMIZE_BASE, and PLTs may be necessary for B/BL.

We should have removed the check for !CONFIG_RANDOMIZE_BASE as part of that.

> >  arch/arm64/kernel/module-plts.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/module-plts.c 
> > b/arch/arm64/kernel/module-plts.c
> > index bd69a4e7cd60..21a67d52d7a0 100644
> > --- a/arch/arm64/kernel/module-plts.c
> > +++ b/arch/arm64/kernel/module-plts.c
> > @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, 
> > Elf64_Rela *rela, int num,
> >  		switch (ELF64_R_TYPE(rela[i].r_info)) {
> >  		case R_AARCH64_JUMP26:
> >  		case R_AARCH64_CALL26:
> > -			if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> > -				break;
> > -
> >  			/*
> >  			 * We only have to consider branch targets that resolve
> >  			 * to symbols that are defined in a different section.
> 
> I see there are two such checks (in partition_branch_plt_relas()
> and in count_plts()), can you explain in more detail how you
> concluded that one of them is correct but the other one is not?

I believe that the one in partition_branch_plt_relas() needs to go too; that's
just a minor optimization for the case where there shouldn't be any PLTs for
B/BL, and it no longer holds after the module VA range rework.

That was introduced in commit:

  d4e0340919fb9190 ("arm64/module: Optimize module load time by optimizing PLT counting")

Thanks,
Mark.

Re: [PATCH] arm64: module: PLT allowed even !RANDOM_BASE

Posted by Aiqun(Maria) Yu 2 years, 3 months ago

On 10/23/2023 5:02 PM, Mark Rutland wrote:
> On Mon, Oct 23, 2023 at 10:08:33AM +0200, Arnd Bergmann wrote:
>> On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote:
>>> Module PLT feature can be enabled even when RANDOM_BASE is disabled.
>>> Break BLT entry counts of relocation types will make module plt entry
>>> allocation fail and finally exec format error for even correct and plt
>>> allocation available modules.
> 
> Has an actual problem been seen in practice, or was this found by looking at
> the code?
I've encounter an actual problem when disalbe CONFIG_RADOM_BASE and the 
kernel module have the exec format error issue.
> 
>>>
>>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
>>
>> Adding Ard Biesheuvel to Cc, as he added the check in commit
>> a257e02579e42 ("arm64/kernel: don't ban ADRP to work around
>> Cortex-A53 erratum #843419")
Thx for adding Ard. Will keep him in next patchset as well.
> 
> I think that the actual mistake is in commit:
> 
>    3e35d303ab7d22c4 ("arm64: module: rework module VA range selection")
> 
> Prior to that commit, when CONFIG_RANDOMIZE_BASE=n all modules and code had to
> be within 128M of each other, and so there were no PLTs necessary for B/BL.
> After that commit we can have a 2G module range regardless of
> CONFIG_RANDOMIZE_BASE, and PLTs may be necessary for B/BL.
> 
> We should have removed the check for !CONFIG_RANDOMIZE_BASE as part of that.

Agree with you.
> 
>>>   arch/arm64/kernel/module-plts.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/module-plts.c
>>> b/arch/arm64/kernel/module-plts.c
>>> index bd69a4e7cd60..21a67d52d7a0 100644
>>> --- a/arch/arm64/kernel/module-plts.c
>>> +++ b/arch/arm64/kernel/module-plts.c
>>> @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms,
>>> Elf64_Rela *rela, int num,
>>>   		switch (ELF64_R_TYPE(rela[i].r_info)) {
>>>   		case R_AARCH64_JUMP26:
>>>   		case R_AARCH64_CALL26:
>>> -			if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
>>> -				break;
>>> -
>>>   			/*
>>>   			 * We only have to consider branch targets that resolve
>>>   			 * to symbols that are defined in a different section.
>>
>> I see there are two such checks (in partition_branch_plt_relas()
>> and in count_plts()), can you explain in more detail how you
>> concluded that one of them is correct but the other one is not?
> 
> I believe that the one in partition_branch_plt_relas() needs to go too; that's
> just a minor optimization for the case where there shouldn't be any PLTs for
> B/BL, and it no longer holds after the module VA range rework.
> 
> That was introduced in commit:
> 
>    d4e0340919fb9190 ("arm64/module: Optimize module load time by optimizing PLT counting")
The functionality is the same from my try with plt allocated kernel 
modules. While the PLT entry can be dramatically reduced from ~50000 to 
~500 after fix in partition_branch_plt_relas.

I will send out the second patchset with fix in 
partition_branch_plt_relas (remove check of CONFIG_RANDOMIZE_BASE) 
tomorrow if no more other comments today.
> 
> Thanks,
> Mark.

-- 
Thx and BRs,
Aiqun(Maria) Yu