[PATCH 0/2] Fix objtool warning about do_syscall() on LoongArch

[PATCH 0/2] Fix objtool warning about do_syscall() on LoongArch

[Tiezhu Yang]

Tiezhu Yang (2):
  objtool/LoongArch: Restrict stack operation instruction
  LoongArch: Remove STACK_FRAME_NON_STANDARD(do_syscall)

Tiezhu Yang (2):
  objtool/LoongArch: Restrict stack operation instruction
  LoongArch: Remove STACK_FRAME_NON_STANDARD(do_syscall)

 arch/loongarch/kernel/syscall.c       | 4 ----
 tools/objtool/arch/loongarch/decode.c | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

-- 
2.42.0

[PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Tiezhu Yang]

After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
support"), the code flow of do_syscall() was changed when compiled
with GCC due to the secondary stack of add_random_kstack_offset(),
something like this:

  addi.d          $sp, $sp, -32
  st.d            $fp, $sp, 16
  st.d            $ra, $sp, 24
  addi.d          $fp, $sp, 32
  ...
  sub.d           $sp, $sp, $t1
  ...
  addi.d          $sp, $fp, -32
  ld.d            $ra, $sp, 24
  ld.d            $fp, $sp, 16
  addi.d          $sp, $sp, 32

fp points to the stack top, it is only used to save and restore the
original sp and is not used as cfa base for arch_callee_saved_reg().
In the case OP_SRC_ADD of update_cfi_state(), the above rare case is
not handled so that lead to a wrong stack size, then there exists a
objtool warning "do_syscall+0x11c: return with modified stack frame".

Because the fp related instructions do not modify the stack frame,
no need to decode them, just restrict stack operation instruction
only with the single case "addi.d sp,sp,si12".

By the way, if fp is used as cfa base for arch_callee_saved_reg()
(there is no this behavior on LoongArch at present), then it needs
to decode the related instructions and modify update_cfi_state().

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407201336.mW8dj1VB-lkp@intel.com/
Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/arch/loongarch/decode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
index aee479d2191c..6a34af675cee 100644
--- a/tools/objtool/arch/loongarch/decode.c
+++ b/tools/objtool/arch/loongarch/decode.c
@@ -121,8 +121,8 @@ static bool decode_insn_reg2i12_fomat(union loongarch_instruction inst,
 {
 	switch (inst.reg2i12_format.opcode) {
 	case addid_op:
-		if ((inst.reg2i12_format.rd == CFI_SP) || (inst.reg2i12_format.rj == CFI_SP)) {
-			/* addi.d sp,sp,si12 or addi.d fp,sp,si12 */
+		if ((inst.reg2i12_format.rd == CFI_SP) && (inst.reg2i12_format.rj == CFI_SP)) {
+			/* addi.d sp,sp,si12 */
 			insn->immediate = sign_extend64(inst.reg2i12_format.immediate, 11);
 			ADD_OP(op) {
 				op->src.type = OP_SRC_ADD;
-- 
2.42.0

[PATCH 2/2] LoongArch: Remove STACK_FRAME_NON_STANDARD(do_syscall)

[Tiezhu Yang]

For now, it can remove STACK_FRAME_NON_STANDARD(do_syscall) because
there is no objtool warning "do_syscall+0x11c: return with modified
stack frame".

Fixes: a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET support")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/kernel/syscall.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/loongarch/kernel/syscall.c b/arch/loongarch/kernel/syscall.c
index ba5d0930a74f..168bd97540f8 100644
--- a/arch/loongarch/kernel/syscall.c
+++ b/arch/loongarch/kernel/syscall.c
@@ -79,7 +79,3 @@ void noinstr __no_stack_protector do_syscall(struct pt_regs *regs)
 
 	syscall_exit_to_user_mode(regs);
 }
-
-#ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
-STACK_FRAME_NON_STANDARD(do_syscall);
-#endif
-- 
2.42.0

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Huacai Chen]

Hi, Tiezhu,

Can this patch also fix the warnings of ClangBuiltLinux?

Huacai

On Tue, Jul 30, 2024 at 2:19 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
> support"), the code flow of do_syscall() was changed when compiled
> with GCC due to the secondary stack of add_random_kstack_offset(),
> something like this:
>
>   addi.d          $sp, $sp, -32
>   st.d            $fp, $sp, 16
>   st.d            $ra, $sp, 24
>   addi.d          $fp, $sp, 32
>   ...
>   sub.d           $sp, $sp, $t1
>   ...
>   addi.d          $sp, $fp, -32
>   ld.d            $ra, $sp, 24
>   ld.d            $fp, $sp, 16
>   addi.d          $sp, $sp, 32
>
> fp points to the stack top, it is only used to save and restore the
> original sp and is not used as cfa base for arch_callee_saved_reg().
> In the case OP_SRC_ADD of update_cfi_state(), the above rare case is
> not handled so that lead to a wrong stack size, then there exists a
> objtool warning "do_syscall+0x11c: return with modified stack frame".
>
> Because the fp related instructions do not modify the stack frame,
> no need to decode them, just restrict stack operation instruction
> only with the single case "addi.d sp,sp,si12".
>
> By the way, if fp is used as cfa base for arch_callee_saved_reg()
> (there is no this behavior on LoongArch at present), then it needs
> to decode the related instructions and modify update_cfi_state().
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407201336.mW8dj1VB-lkp@intel.com/
> Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  tools/objtool/arch/loongarch/decode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
> index aee479d2191c..6a34af675cee 100644
> --- a/tools/objtool/arch/loongarch/decode.c
> +++ b/tools/objtool/arch/loongarch/decode.c
> @@ -121,8 +121,8 @@ static bool decode_insn_reg2i12_fomat(union loongarch_instruction inst,
>  {
>         switch (inst.reg2i12_format.opcode) {
>         case addid_op:
> -               if ((inst.reg2i12_format.rd == CFI_SP) || (inst.reg2i12_format.rj == CFI_SP)) {
> -                       /* addi.d sp,sp,si12 or addi.d fp,sp,si12 */
> +               if ((inst.reg2i12_format.rd == CFI_SP) && (inst.reg2i12_format.rj == CFI_SP)) {
> +                       /* addi.d sp,sp,si12 */
>                         insn->immediate = sign_extend64(inst.reg2i12_format.immediate, 11);
>                         ADD_OP(op) {
>                                 op->src.type = OP_SRC_ADD;
> --
> 2.42.0
>
>

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Jinyang He]

On 2024-07-30 14:19, Tiezhu Yang wrote:

> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
> support"), the code flow of do_syscall() was changed when compiled
> with GCC due to the secondary stack of add_random_kstack_offset(),
> something like this:
>
>    addi.d          $sp, $sp, -32
>    st.d            $fp, $sp, 16
>    st.d            $ra, $sp, 24
>    addi.d          $fp, $sp, 32
>    ...
>    sub.d           $sp, $sp, $t1
Have you checked the ORC info whether is right or tried backtrace which
passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
$sp cannot express CFA here. This patch just clear the warning but ignore
the validity of ORC info. The wrong ORC info may cause illegally access
memory when backtrace.


Thanks,
Jinyang
>    ...
>    addi.d          $sp, $fp, -32
>    ld.d            $ra, $sp, 24
>    ld.d            $fp, $sp, 16
>    addi.d          $sp, $sp, 32
>
> fp points to the stack top, it is only used to save and restore the
> original sp and is not used as cfa base for arch_callee_saved_reg().
> In the case OP_SRC_ADD of update_cfi_state(), the above rare case is
> not handled so that lead to a wrong stack size, then there exists a
> objtool warning "do_syscall+0x11c: return with modified stack frame".
>
> Because the fp related instructions do not modify the stack frame,
> no need to decode them, just restrict stack operation instruction
> only with the single case "addi.d sp,sp,si12".
>
> By the way, if fp is used as cfa base for arch_callee_saved_reg()
> (there is no this behavior on LoongArch at present), then it needs
> to decode the related instructions and modify update_cfi_state().
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407201336.mW8dj1VB-lkp@intel.com/
> Fixes: b2d23158e6c8 ("objtool/LoongArch: Implement instruction decoder")
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   tools/objtool/arch/loongarch/decode.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/objtool/arch/loongarch/decode.c b/tools/objtool/arch/loongarch/decode.c
> index aee479d2191c..6a34af675cee 100644
> --- a/tools/objtool/arch/loongarch/decode.c
> +++ b/tools/objtool/arch/loongarch/decode.c
> @@ -121,8 +121,8 @@ static bool decode_insn_reg2i12_fomat(union loongarch_instruction inst,
>   {
>   	switch (inst.reg2i12_format.opcode) {
>   	case addid_op:
> -		if ((inst.reg2i12_format.rd == CFI_SP) || (inst.reg2i12_format.rj == CFI_SP)) {
> -			/* addi.d sp,sp,si12 or addi.d fp,sp,si12 */
> +		if ((inst.reg2i12_format.rd == CFI_SP) && (inst.reg2i12_format.rj == CFI_SP)) {
> +			/* addi.d sp,sp,si12 */
>   			insn->immediate = sign_extend64(inst.reg2i12_format.immediate, 11);
>   			ADD_OP(op) {
>   				op->src.type = OP_SRC_ADD;

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Tiezhu Yang]

On 07/30/2024 05:28 PM, Jinyang He wrote:
> On 2024-07-30 14:19, Tiezhu Yang wrote:
>
>> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
>> support"), the code flow of do_syscall() was changed when compiled
>> with GCC due to the secondary stack of add_random_kstack_offset(),
>> something like this:
>>
>>    addi.d          $sp, $sp, -32
>>    st.d            $fp, $sp, 16
>>    st.d            $ra, $sp, 24
>>    addi.d          $fp, $sp, 32
>>    ...
>>    sub.d           $sp, $sp, $t1
> Have you checked the ORC info whether is right or tried backtrace which
> passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
> $sp cannot express CFA here. This patch just clear the warning but ignore
> the validity of ORC info. The wrong ORC info may cause illegally access
> memory when backtrace.

I did testing many times before submitting, the call trace is
expected when testing "echo l > /proc/sysrq-trigger".

Thanks,
Tiezhu

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Jinyang He]

On 2024-07-30 17:49, Tiezhu Yang wrote:
> On 07/30/2024 05:28 PM, Jinyang He wrote:
>> On 2024-07-30 14:19, Tiezhu Yang wrote:
>>
>>> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
>>> support"), the code flow of do_syscall() was changed when compiled
>>> with GCC due to the secondary stack of add_random_kstack_offset(),
>>> something like this:
>>>
>>>    addi.d          $sp, $sp, -32
>>>    st.d            $fp, $sp, 16
>>>    st.d            $ra, $sp, 24
>>>    addi.d          $fp, $sp, 32
>>>    ...
>>>    sub.d           $sp, $sp, $t1
>> Have you checked the ORC info whether is right or tried backtrace which
>> passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
>> $sp cannot express CFA here. This patch just clear the warning but 
>> ignore
>> the validity of ORC info. The wrong ORC info may cause illegally access
>> memory when backtrace.
>
> I did testing many times before submitting, the call trace is
> expected when testing "echo l > /proc/sysrq-trigger".
Make sure the RANDOMIZE_KSTACK_OFFSET is enable. I tested it by
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y on qemu and it did
not show the frame about handle_syscall.

On the other hand,
$ ./tools/objtool/objtool --dump arch/loongarch/kernel/syscall.o
.noinstr.text+0:type:call sp:sp +   0 fp: (und)  ra: (und)  signal:0
.noinstr.text+4:type:call sp:sp +  32 fp: (und)  ra: (und)  signal:0
.noinstr.text+8:type:call sp:sp +  32 fp:prevsp + -16 ra: (und) signal:0
.noinstr.text+c:type:call sp:sp +  32 fp:prevsp + -16 ra:prevsp + -8 
signal:0
.noinstr.text+e4:type:call sp:sp +  32 fp:prevsp + -16 ra: (und) signal:0
.noinstr.text+e8:type:call sp:sp +  32 fp: (und)  ra: (und) signal:0
.noinstr.text+ec:type:call sp:sp +   0 fp: (und)  ra: (und) signal:0
.noinstr.text+f0:type:? sp: (und)  fp: (und)  ra: (und)  signal:0
.noinstr.text+100:type:call sp:sp +  32 fp:prevsp + -16 ra:prevsp + -8 
signal:0
.noinstr.text+118:type:? sp: (und)  fp: (und)  ra: (und)  signal:0
$ objdump -d arch/loongarch/kernel/syscall.o | grep "sub.*sp.*sp"
   70:    0011b463     sub.d           $sp, $sp, $t1
Obviously the ORC info is wrong.
(Assume the backtrace PC is 0x74, how to find the previous frame?)

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Tiezhu Yang]

On 7/30/24 18:57, Jinyang He wrote:
> 
> On 2024-07-30 17:49, Tiezhu Yang wrote:
>> On 07/30/2024 05:28 PM, Jinyang He wrote:
>>> On 2024-07-30 14:19, Tiezhu Yang wrote:
>>>
>>>> After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
>>>> support"), the code flow of do_syscall() was changed when compiled
>>>> with GCC due to the secondary stack of add_random_kstack_offset(),
>>>> something like this:
>>>>
>>>>    addi.d          $sp, $sp, -32
>>>>    st.d            $fp, $sp, 16
>>>>    st.d            $ra, $sp, 24
>>>>    addi.d          $fp, $sp, 32
>>>>    ...
>>>>    sub.d           $sp, $sp, $t1
>>> Have you checked the ORC info whether is right or tried backtrace which
>>> passed do_syscall? The "sub.d $sp, $sp, $t1" has modified the $sp so the
>>> $sp cannot express CFA here. This patch just clear the warning but 
>>> ignore
>>> the validity of ORC info. The wrong ORC info may cause illegally access
>>> memory when backtrace.
>>
>> I did testing many times before submitting, the call trace is
>> expected when testing "echo l > /proc/sysrq-trigger".
> Make sure the RANDOMIZE_KSTACK_OFFSET is enable. I tested it by
> CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y on qemu and it did
> not show the frame about handle_syscall.

I tested with the defconfig, CONFIG_RANDOMIZE_KSTACK_OFFSET is set but
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT is not set, the call trace has
handle_syscall() which is the previous frame of do_syscall(), and the
orc dump info is correct.

Let me modify the config file and test again with the following configs:
CONFIG_RANDOMIZE_KSTACK_OFFSET=y CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y

If there exists problem, I will fix it.

Thanks,
Tiezhu

Re: [PATCH 1/2] objtool/LoongArch: Restrict stack operation instruction

[Tiezhu Yang]

On 7/30/24 16:59, Huacai Chen wrote:
> Hi, Tiezhu,
> 
> Can this patch also fix the warnings of ClangBuiltLinux?

For now, objtool is only built with GCC, I will try to build with
Clang after the known issues are solved.

Thanks,
Tiezhu