linux-kernel - Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2023 19:00:20 +0800
From:   WANG Xuerui <kernel@...0n.name>
To:     Xi Ruoyao <xry111@...111.site>, Qing Zhang <zhangqing@...ngson.cn>,
        Huacai Chen <chenhuacai@...nel.org>
Cc:     Jiaxun Yang <jiaxun.yang@...goat.com>, loongarch@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit
 alignment

On 2023/4/19 18:42, Xi Ruoyao wrote:
> On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
>> This is done in order to easily calculate the number of breakpoints
>> in hw_break_get.
>>
>> Signed-off-by: Qing Zhang <zhangqing@...ngson.cn>
>> ---
>>   arch/loongarch/include/uapi/asm/ptrace.h |  3 ++-
>>   arch/loongarch/kernel/ptrace.c           | 13 +++++++++----
>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
>> b/arch/loongarch/include/uapi/asm/ptrace.h
>> index 2282ae1fd3b6..06e3be52cb04 100644
>> --- a/arch/loongarch/include/uapi/asm/ptrace.h
>> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
>> @@ -57,11 +57,12 @@ struct user_lasx_state {

Drive-by comment to the patch author: there is no "user_lasx_state" yet. 
Please always state your base commit if not obvious, or you should start 
from some well-known upstream HEAD (e.g. Linus' rc tags, 
loongarch-fixes, or loongarch-next).

>>   };
>>   
>>   struct user_watch_state {
>> -       uint16_t dbg_info;
>> +       uint64_t dbg_info;
> 
> Ouch.  This is a breaking change when we consider user code like
> `printf(PRIu16 "\n", ptr->dbg_info);`.  Is it really necessary?

Ah right. This is UAPI so without *very* concrete and convicing reason 
why the change is not going to impact any potential users, it's gonna be 
a presumed NAK. In other words you must demonstrate (1) why it's 
absolutely necessary to make the change and (2) that it's impossible to 
impact anyone, before any such changes can even be considered.

> 
>>          struct {
>>                  uint64_t    addr;
>>                  uint64_t    mask;
>>                  uint32_t    ctrl;
>> +               uint32_t    pad;
>>          } dbg_regs[8];
>>   };
>>   
>> diff --git a/arch/loongarch/kernel/ptrace.c
>> b/arch/loongarch/kernel/ptrace.c
>> index 0c7c41e41cad..9c3bc1bbf2ff 100644
>> --- a/arch/loongarch/kernel/ptrace.c
>> +++ b/arch/loongarch/kernel/ptrace.c
>> @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
>> int note_type,
>>          return 0;
>>   }
>>   
>> -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
>> *info)
>> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
>> *info)
>>   {
>>          u8 num;
>> -       u16 reg = 0;
>> +       u64 reg = 0;
>>   
>>          switch (note_type) {
>>          case NT_LOONGARCH_HW_BREAK:
>> @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
>> *target,
>>                          const struct user_regset *regset,
>>                          struct membuf to)
>>   {
>> -       u16 info;
>> +       u64 info;
>>          u32 ctrl;
>>          u64 addr, mask;
>>          int ret, idx = 0;
>> @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
>> *target,
>>                  membuf_store(&to, addr);
>>                  membuf_store(&to, mask);
>>                  membuf_store(&to, ctrl);
>> +               membuf_zero(&to, sizeof(u32));
>>                  idx++;
>>          }
>>   
>> @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
>> *target,
>>          int ret, idx = 0, offset, limit;
>>          unsigned int note_type = regset->core_note_type;
>>   
>> -       /* Resource info */
>> +       /* Resource info and pad */
>>          offset = offsetof(struct user_watch_state, dbg_regs);
>>          user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
>> offset);
>>   
>> @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
>> *target,
>>                  if (ret)
>>                          return ret;
>>                  offset += PTRACE_HBP_CTRL_SZ;
>> +
>> +               user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
>> +                                         offset, offset +
>> PTRACE_HBP_PAD_SZ);
>> +               offset += PTRACE_HBP_PAD_SZ;
>>                  idx++;
>>          }
>>   
> 

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/