| Message ID | 20241211-arm64-gcs-signal-sparse-v2-1-c22f37216135@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] arm64/signal: Silence sparse warning storing GCSPR_EL0 | expand |
On Wed, Dec 11, 2024 at 01:00:35AM +0000, Mark Brown wrote: > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index 14ac6fdb872b9672e4b16a097f1b577aae8dec50..08d51fabdb9d47c848f14c9b25d6be04f109c2ee 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -39,7 +39,7 @@ > #ifdef CONFIG_ARM64_GCS > #define GCS_SIGNAL_CAP(addr) (((unsigned long)addr) & GCS_CAP_ADDR_MASK) > > -static bool gcs_signal_cap_valid(u64 addr, u64 val) > +static bool gcs_signal_cap_valid(unsigned long __user *addr, u64 val) > { > return val == GCS_SIGNAL_CAP(addr); > } Another personal preference - addresses should be (unsigned long), pointer to be accessed (... __user *). But we could even scrap this function, there's a single caller to a one-line function. > @@ -1094,15 +1094,15 @@ static int gcs_restore_signal(void) > /* > * Check that the cap is the actual GCS before replacing it. > */ > - if (!gcs_signal_cap_valid((u64)gcspr_el0, cap)) > + if (!gcs_signal_cap_valid(gcspr_el0, cap)) > return -EINVAL; > > /* Invalidate the token to prevent reuse */ > - put_user_gcs(0, (__user void*)gcspr_el0, &ret); > + put_user_gcs(0, gcspr_el0, &ret); > if (ret != 0) > return -EFAULT; > > - write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0); > + write_sysreg_s((__force u64)(gcspr_el0 + 1), SYS_GCSPR_EL0); > > return 0; > } Looking through the code, do we have a similar problem in gcs_signal_entry()? Or do we rely on sparse ignoring (unsigned long) casts? Whichever way we go, I think we should have consistency between these two functions.
On Fri, Dec 13, 2024 at 04:26:40PM +0000, Catalin Marinas wrote: > On Wed, Dec 11, 2024 at 01:00:35AM +0000, Mark Brown wrote: > > -static bool gcs_signal_cap_valid(u64 addr, u64 val) > > +static bool gcs_signal_cap_valid(unsigned long __user *addr, u64 val) > > { > > return val == GCS_SIGNAL_CAP(addr); > > } > Another personal preference - addresses should be (unsigned long), > pointer to be accessed (... __user *). But we could even scrap this > function, there's a single caller to a one-line function. I'm not sure I follow what you're saying here - the pointer here is an unsigned long __user *? The value in val is not a pointer or address, it's a cap value derived from a pointer. But yeah, we could inline and render it moot. > > - write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0); > > + write_sysreg_s((__force u64)(gcspr_el0 + 1), SYS_GCSPR_EL0); > Looking through the code, do we have a similar problem in > gcs_signal_entry()? Or do we rely on sparse ignoring (unsigned long) > casts? > Whichever way we go, I think we should have consistency between these > two functions. It's not coming up since there's a cast to unsigned long in there which sparse likes, I can adjust that to a cast to __force u64 in there since people didn't seem to like the cast to unsigned long.
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 14ac6fdb872b9672e4b16a097f1b577aae8dec50..08d51fabdb9d47c848f14c9b25d6be04f109c2ee 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -39,7 +39,7 @@ #ifdef CONFIG_ARM64_GCS #define GCS_SIGNAL_CAP(addr) (((unsigned long)addr) & GCS_CAP_ADDR_MASK) -static bool gcs_signal_cap_valid(u64 addr, u64 val) +static bool gcs_signal_cap_valid(unsigned long __user *addr, u64 val) { return val == GCS_SIGNAL_CAP(addr); } @@ -1094,15 +1094,15 @@ static int gcs_restore_signal(void) /* * Check that the cap is the actual GCS before replacing it. */ - if (!gcs_signal_cap_valid((u64)gcspr_el0, cap)) + if (!gcs_signal_cap_valid(gcspr_el0, cap)) return -EINVAL; /* Invalidate the token to prevent reuse */ - put_user_gcs(0, (__user void*)gcspr_el0, &ret); + put_user_gcs(0, gcspr_el0, &ret); if (ret != 0) return -EFAULT; - write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0); + write_sysreg_s((__force u64)(gcspr_el0 + 1), SYS_GCSPR_EL0); return 0; }
We are seeing a sparse warning in gcs_restore_signal(): arch/arm64/kernel/signal.c:1054:9: sparse: sparse: cast removes address space '__user' of expression when storing the final GCSPR_EL0 value back into the register, caused by the fact that write_sysreg_s() casts the value it writes to a u64 which sparse sees as discarding the __userness of the pointer. Add a __force cast to tell sparse that this is intentional. While we're at it also remove spurious casts of the gcspr_el0 value as we manipulate it which were the result of bitrot as the code was tweaked in the long period it was out of tree. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202412082005.OBJ0BbWs-lkp@intel.com/ Signed-off-by: Mark Brown <broonie@kernel.org> --- Changes in v2: - Use __force u64 rather than unsigned long. - Tweak commit message. - Link to v1: https://lore.kernel.org/r/20241210-arm64-gcs-signal-sparse-v1-1-26888bcd6f89@kernel.org --- arch/arm64/kernel/signal.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 change-id: 20241209-arm64-gcs-signal-sparse-53fa9cad67f7 Best regards,