linux-kernel - Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '_

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]

Message-ID: <Z1cOs2sGff1_TtQZ@arm.com>
Date: Mon, 9 Dec 2024 15:37:23 +0000
From: Catalin Marinas <catalin.marinas@....com>
To: kernel test robot <lkp@...el.com>
Cc: Mark Brown <broonie@...nel.org>, oe-kbuild-all@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Thiago Jung Bauermann <thiago.bauermann@...aro.org>
Subject: Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes
 address space '__user' of expression

On Mon, Dec 09, 2024 at 12:47:33PM +0800, kernel test robot wrote:
> sparse warnings: (new ones prefixed by >>)
> >> arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression
>    arch/arm64/kernel/signal.c:1054:9: sparse: sparse: cast removes address space '__user' of expression
>    arch/arm64/kernel/signal.c:1461:26: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... ) @@     got void * @@
>    arch/arm64/kernel/signal.c:1461:26: sparse:     expected void ( [noderef] [usertype] __user *[assigned] [usertype] sigtramp )( ... )
>    arch/arm64/kernel/signal.c:1461:26: sparse:     got void *
> 
> vim +/__user +1046 arch/arm64/kernel/signal.c

Thanks for this, the warning is valid.

> 2c020ed8d148f7 Catalin Marinas 2012-03-05  1010  
> eaf62ce1563b85 Mark Brown      2024-10-01  1011  #ifdef CONFIG_ARM64_GCS
> eaf62ce1563b85 Mark Brown      2024-10-01  1012  static int gcs_restore_signal(void)
> eaf62ce1563b85 Mark Brown      2024-10-01  1013  {
> eaf62ce1563b85 Mark Brown      2024-10-01  1014  	unsigned long __user *gcspr_el0;

I think we should keep this as u64 since it's a sysreg.

> eaf62ce1563b85 Mark Brown      2024-10-01  1015  	u64 cap;
> eaf62ce1563b85 Mark Brown      2024-10-01  1016  	int ret;
> eaf62ce1563b85 Mark Brown      2024-10-01  1017  
> eaf62ce1563b85 Mark Brown      2024-10-01  1018  	if (!system_supports_gcs())
> eaf62ce1563b85 Mark Brown      2024-10-01  1019  		return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1020  
> eaf62ce1563b85 Mark Brown      2024-10-01  1021  	if (!(current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE))
> eaf62ce1563b85 Mark Brown      2024-10-01  1022  		return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1023  
> eaf62ce1563b85 Mark Brown      2024-10-01  1024  	gcspr_el0 = (unsigned long __user *)read_sysreg_s(SYS_GCSPR_EL0);

Remove the cast here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1025  
> eaf62ce1563b85 Mark Brown      2024-10-01  1026  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1027  	 * Ensure that any changes to the GCS done via GCS operations
> eaf62ce1563b85 Mark Brown      2024-10-01  1028  	 * are visible to the normal reads we do to validate the
> eaf62ce1563b85 Mark Brown      2024-10-01  1029  	 * token.
> eaf62ce1563b85 Mark Brown      2024-10-01  1030  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01  1031  	gcsb_dsync();
> eaf62ce1563b85 Mark Brown      2024-10-01  1032  
> eaf62ce1563b85 Mark Brown      2024-10-01  1033  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1034  	 * GCSPR_EL0 should be pointing at a capped GCS, read the cap.
> eaf62ce1563b85 Mark Brown      2024-10-01  1035  	 * We don't enforce that this is in a GCS page, if it is not
> eaf62ce1563b85 Mark Brown      2024-10-01  1036  	 * then faults will be generated on GCS operations - the main
> eaf62ce1563b85 Mark Brown      2024-10-01  1037  	 * concern is to protect GCS pages.
> eaf62ce1563b85 Mark Brown      2024-10-01  1038  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01  1039  	ret = copy_from_user(&cap, gcspr_el0, sizeof(cap));

Add one here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1040  	if (ret)
> eaf62ce1563b85 Mark Brown      2024-10-01  1041  		return -EFAULT;
> eaf62ce1563b85 Mark Brown      2024-10-01  1042  
> eaf62ce1563b85 Mark Brown      2024-10-01  1043  	/*
> eaf62ce1563b85 Mark Brown      2024-10-01  1044  	 * Check that the cap is the actual GCS before replacing it.
> eaf62ce1563b85 Mark Brown      2024-10-01  1045  	 */
> eaf62ce1563b85 Mark Brown      2024-10-01 @1046  	if (!gcs_signal_cap_valid((u64)gcspr_el0, cap))

Drop the cast here.

> eaf62ce1563b85 Mark Brown      2024-10-01  1047  		return -EINVAL;
> eaf62ce1563b85 Mark Brown      2024-10-01  1048  
> eaf62ce1563b85 Mark Brown      2024-10-01  1049  	/* Invalidate the token to prevent reuse */
> eaf62ce1563b85 Mark Brown      2024-10-01  1050  	put_user_gcs(0, (__user void*)gcspr_el0, &ret);

We need a cast here if we are to go with u64 gcspr_el0 (it wasn't needed
before, not sure why it was cast to void *).

> eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
> eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
> eaf62ce1563b85 Mark Brown      2024-10-01  1053  
> eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);

And this would be +8 I guess.

> eaf62ce1563b85 Mark Brown      2024-10-01  1055  
> eaf62ce1563b85 Mark Brown      2024-10-01  1056  	return 0;
> eaf62ce1563b85 Mark Brown      2024-10-01  1057  }
> eaf62ce1563b85 Mark Brown      2024-10-01  1058  

-- 
Catalin