Re: [PATCH] riscv: Disallow PR_GET_TAGGED_ADDR_CTRL without Supm

[Palmer Dabbelt]

On Wed, 07 May 2025 11:08:11 PDT (-0700), namcao@xxxxxxxxxxxxx wrote:
> On Wed, May 07, 2025 at 07:52:18AM -0700, Samuel Holland wrote:
> > When the prctl() interface for pointer masking was added, it did not
> > check that the pointer masking ISA extension was supported, only the
> > individual submodes. Userspace could still attempt to disable pointer
> > masking and query the pointer masking state. commit 81de1afb2dd1
> > ("riscv: Fix kernel crash due to PR_SET_TAGGED_ADDR_CTRL") disallowed
> > the former, as the senvcfg write could crash on older systems.
> > PR_GET_TAGGED_ADDR_CTRL state does not crash, because it reads only
> > kernel-internal state and not senvcfg, but it should still be disallowed
> > for consistency.

We talked some in the patchwork meeting about returning success for the 
flavors of this that could be emulated in the kernel without hardware 
support (ie, disabling pointer masking on systems that don't have it is 
trivial).  The general consensus is that didn't really help any, as 
userspace still needs to deal with systems where these will fail (old 
kernels, Kconfig disabled, etc).

Happy to reconsider that if someone cares.  Either way this is going up 
as a fix, as it just keeps the interface sane when taking 7f1c3de1370b 
("riscv: Disallow PR_GET_TAGGED_ADDR_CTRL without Supm") into account.

> > Fixes: 09d6775f503b ("riscv: Add support for userspace pointer masking")
> > Signed-off-by: Samuel Holland <samuel.holland@xxxxxxxxxx>
> > ---
> >
> >  arch/riscv/kernel/process.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> > index 7c244de77180..f7a1a887ae68 100644
> > --- a/arch/riscv/kernel/process.c
> > +++ b/arch/riscv/kernel/process.c
> > @@ -330,6 +330,9 @@ long get_tagged_addr_ctrl(struct task_struct *task)
> >  	struct thread_info *ti = task_thread_info(task);
> >  	long ret = 0;
> >
> > +	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
> > +		return -EINVAL;
> > +
> >  	if (is_compat_thread(ti))
> >  		return -EINVAL;
>
> I think this matches what the man page says:
>
> "If the arguments are invalid or this feature is disabled or unsupported by
> the kernel, the call fails with EINVAL"

Ya, it's also what we do when the Kconfig is disabled so it seems like 
the way to go (we talked about that some too).

> Reviewed-by: Nam Cao <namcao@xxxxxxxxxxxxx>

Thanks!