linux-kernel - Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints

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] [day] [month] [year] [list]

Message-ID: <0943c7f0-65e1-e3bf-97e1-10125c46d316@loongson.cn>
Date: Wed, 22 Jan 2025 11:09:24 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Xi Ruoyao <xry111@...111.site>, WANG Xuerui <kernel@...0n.name>,
 Huacai Chen <chenhuacai@...nel.org>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints

On 01/21/2025 09:46 PM, Xi Ruoyao wrote:
> On Tue, 2025-01-21 at 21:35 +0800, WANG Xuerui wrote:
>> one cannot account for older (in fact, *every existing*) clients
>> expecting the old UAPI, hence providing a smaller buffer than struct
>> user_watch_state_v2. In which case the kernel will happily write
>> out-of-bounds if the client asks for the current watchpoint state. This
>> is not acceptable I'm afraid.
>
> Yes, many distros support running the latest kernel with an old
> userspace for hardware compatibility reason and the change will blow up
> gdb on all those distros.  We need to do something smarter to retain the
> backward compatibility.

Hi Xuerui and Ruoyao,

The compatibility problem has been considered when developing and testing
the patches, when the applications in the userspace get watchpoint state,
the length will be specified which will not bigger than the sizeof struct
user_watch_state or user_watch_state_v2, the actual length is assigned as
the minimal value of the application and kernel in the generic ptrace:

kernel/ptrace.c: ptrace_regset():

	kiov->iov_len = min(kiov->iov_len,
			            (__kernel_size_t) (regset->n * regset->size));

	if (req == PTRACE_GETREGSET)
		    return copy_regset_to_user(task, view, regset_no, 0,
					                   kiov->iov_len, kiov->iov_base);
	else
		    return copy_regset_from_user(task, view, regset_no, 0,
					                     kiov->iov_len, kiov->iov_base);

For example, there are four kind of combinations, all of them work well.

(1) "older kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200;
(2) "newer kernel + newer gdb", the actual length is 8+(8+8+4+4)*14=344;
(3) "older kernel + newer gdb", the actual length is 8+(8+8+4+4)*8=200;
(4) "newer kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200.

If you have more comments, please let me know.


Hi Huacai,

Could you please update the commit message when applying the patches
if you think the changes are OK? Like this:

----------------------
The maximum number of load/store watchpoints and fetch instruction
watchpoints is 14 each according to LoongArch Reference Manual, so
extend the maximum number of watchpoints from 8 to 14 for ptrace.

By the way, just simply change 8 to 14 for the definition in struct
user_watch_state at the beginning, but it may corrupt uapi, then add
a new struct user_watch_state_v2 directly.

As far as I can tell, the only users for this struct in the userspace
are GDB and LLDB, there are no any problems of software compatibility
between the application and kernel according to the analysis.

The compatibility problem has been considered when developing and testing
the patches, when the applications in the userspace get watchpoint state,
the length will be specified which will not bigger than the sizeof struct
user_watch_state or user_watch_state_v2, the actual length is assigned as
the minimal value of the application and kernel in the generic ptrace:

kernel/ptrace.c: ptrace_regset():

	kiov->iov_len = min(kiov->iov_len,
			            (__kernel_size_t) (regset->n * regset->size));

	if (req == PTRACE_GETREGSET)
		    return copy_regset_to_user(task, view, regset_no, 0,
					                   kiov->iov_len, kiov->iov_base);
	else
		    return copy_regset_from_user(task, view, regset_no, 0,
					                     kiov->iov_len, kiov->iov_base);

For example, there are four kind of combinations, all of them work well.

(1) "older kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200;
(2) "newer kernel + newer gdb", the actual length is 8+(8+8+4+4)*14=344;
(3) "older kernel + newer gdb", the actual length is 8+(8+8+4+4)*8=200;
(4) "newer kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200.
----------------------

Thanks,
Tiezhu