Re: [PATCH v10 3/6] arm64: add uaccess to machine check safe

[Tong Tiangen]

在 2024/1/30 20:01, Mark Rutland 写道:
> On Tue, Jan 30, 2024 at 07:14:35PM +0800, Tong Tiangen wrote:
> > 在 2024/1/30 1:43, Mark Rutland 写道:
> > > On Mon, Jan 29, 2024 at 09:46:49PM +0800, Tong Tiangen wrote:
> > > Further, this change will also silently fixup unexpected kernel faults if we
> > > pass bad kernel pointers to copy_{to,from}_user, which will hide real bugs.
> >
> > I think this is better than the panic kernel, because the real bugs
> > belongs to the user process. Even if the wrong pointer is
> > transferred, the page corresponding to the wrong pointer has a memroy
> > error.
>
> I think you have misunderstood my point; I'm talking about the case of a bad
> kernel pointer *without* a memory error.
>
> For example, consider some buggy code such as:
>
> 	void __user *uptr = some_valid_user_pointer;
> 	void *kptr = NULL; // or any other bad pointer
>
> 	ret = copy_to_user(uptr, kptr, size);
> 	if (ret)
> 		return -EFAULT;
>
> Before this patch, when copy_to_user() attempted to load from NULL it would
> fault, there would be no fixup handler for the LDR, and the kernel would die(),
> reporting the bad kernel access.
>
> After this patch (which adds fixup handlers to all the LDR*s in
> copy_to_user()), the fault (which is *not* a memory error) would be handled by
> the fixup handler, and copy_to_user() would return an error without *any*
> indication of the horrible kernel bug.
>
> This will hide kernel bugs, which will make those harder to identify and fix,
> and will also potentially make it easier to exploit the kernel: if the user
> somehow gains control of the kernel pointer, they can rely on the fixup handler
> returning an error, and can scan through memory rather than dying as soon as
> they pas a bad pointer.

I should understand what you mean. I'll think about this and reply.

Many thanks.
Tong.

> > In addition, the panic information contains necessary information
> > for users to check.
>
> There is no panic() in the case I am describing.
>
> > > So NAK to this change as-is; likewise for the addition of USER() to other ldr*
> > > macros in copy_from_user.S and the addition of USER() str* macros in
> > > copy_to_user.S.
> > >
> > > If we want to handle memory errors on some kaccesses, we need a new EX_TYPE_*
> > > separate from the usual EX_TYPE_KACESS_ERR_ZERO that means "handle memory
> > > errors, but treat other faults as fatal". That should come with a rationale and
> > > explanation of why it's actually useful.
> >
> > This makes sense. Add kaccess types that can be processed properly.
> >
> > > [...]
> > >
> > > > diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
> > > > index 478e639f8680..28ec35e3d210 100644
> > > > --- a/arch/arm64/mm/extable.c
> > > > +++ b/arch/arm64/mm/extable.c
> > > > @@ -85,10 +85,10 @@ bool fixup_exception_mc(struct pt_regs *regs)
> > > >    	if (!ex)
> > > >    		return false;
> > > > -	/*
> > > > -	 * This is not complete, More Machine check safe extable type can
> > > > -	 * be processed here.
> > > > -	 */
> > > > +	switch (ex->type) {
> > > > +	case EX_TYPE_UACCESS_ERR_ZERO:
> > > > +		return ex_handler_uaccess_err_zero(ex, regs);
> > > > +	}
> > >
> > > Please fold this part into the prior patch, and start ogf with *only* handling
> > > errors on accesses already marked with EX_TYPE_UACCESS_ERR_ZERO. I think that
> > > change would be relatively uncontroversial, and it would be much easier to
> > > build atop that.
> >
> > OK, the two patches will be merged in the next release.
>
> Thanks.
>
> Mark.
> .