Re: [PATCH] arm64: audit: fix return value high 32bit truncation problem

[Mark Rutland]

Hi,

[adding He Zhe]

On Fri, Jul 30, 2021 at 01:22:37PM +0100, Will Deacon wrote:
> On Thu, Jul 22, 2021 at 02:07:07PM +0800, Yuchen Wei wrote:
> > From: weiyuchen <weiyuchen3@xxxxxxxxxx>
> > 
> > Add error code judgment in invoke_syscall() to prevent kernel
> > components such as audit and tracepoint from obtaining incorrect
> > return values. For example:
> > 
> > type=SYSCALL msg=audit(342.780:69): arch=40000028 syscall=235
> > success=yes exit=4294967235
> > 
> > The syscall return value is -61, but due to the following process in
> > invoke_syscall():
> > 
> > 	if (is_compat_task())
> > 		ret = lower_32_bits(ret);
> > 	regs->regs[0] = ret;
> > 
> > The return value audit or tracepoint get from regs[0] is 4294967235,
> > which is an incorrect return value.
> > 
> > Signed-off-by: weiyuchen <weiyuchen3@xxxxxxxxxx>
> > ---
> >  arch/arm64/kernel/syscall.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index 263d6c1a525f..f9f042d9a088 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -54,7 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
> >  		ret = do_ni_syscall(regs, scno);
> >  	}
> >  
> > -	if (is_compat_task())
> > +	if (is_compat_task() && !IS_ERR_VALUE(ret))
> >  		ret = lower_32_bits(ret);
> 
> Hmm, I'm worried this might break other users who don't expect to see
> non-zero bits for the upper 32-bits of a compat task.
> 
> Mark -- I remember you looking into this relatively recently. Where did you
> get to with it?

Sorry, I've been meaning to chase this up for a bit.

There are a few problems here, but I think we can solve them all (patch
below).

Generally, syscall_get_return_value() should be used to get syscall
return values, and this *should* always sign-extend compat values to 64
bits (but arm64's implementation currently doesn't).

Audit currently uses regs_return_value() rather than
syscall_get_return_value(). That's mostly for historical reasons, but
some architectures don't implement syscall_get_return_value() at the
moment, so we'll have to bodge regs_return_value() on arm64 for now. On
32-bit arm regs_return_value() returns a long, and so is sign-extended.

On 32-bit arm, since syscall_get_return_value() returns a long, it'll
get sign-extended (whether returning an errno or a pointer in the high
2GiB), and we should do the same for compat. We can shuffle
syscall_get_return_value() and syscall_get_error() to handle that.

There are a few places we directly assign to the regs where we need to
truncate the assignment. We can use syscall_set_return_value() to do
that for us.

I think for now, the below should be sufficient for arm64, and we can
chase that up with some cleanup to the core audit code to use
syscall_get_return_value().

Thanks,
Mark.
---->8----