Re: [RFC bpf-next v3 3/5] bpf: Prevent BPF programs from access the buffer pointed by user_optval.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/15/23 10:47 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Since the buffer pointed by ctx->user_optval is in user space, BPF programs
> in kernel space should not access it directly.  They should use
> bpf_copy_from_user() and bpf_copy_to_user() to move data between user and
> kernel space.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/cgroup.c   | 16 +++++++++--
>   kernel/bpf/verifier.c | 66 +++++++++++++++++++++----------------------
>   2 files changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index b977768a28e5..425094e071ba 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -2494,12 +2494,24 @@ static bool cg_sockopt_is_valid_access(int off, int size,
>   	case offsetof(struct bpf_sockopt, optval):
>   		if (size != sizeof(__u64))
>   			return false;
> -		info->reg_type = PTR_TO_PACKET;
> +		if (prog->aux->sleepable)
> +			/* Prohibit access to the memory pointed by optval
> +			 * in sleepable programs.
> +			 */
> +			info->reg_type = PTR_TO_PACKET | MEM_USER;

Is MEM_USER needed to call bpf_copy_from_user()?

Also, from looking at patch 4, the optval could be changed from user memory to 
kernel memory during runtime. Is it useful to check MEM_USER during the verifier 
load time?

How about just return false here to disallow sleepable prog to use ->optval and 
->optval_end. Enforce sleepable prog to stay with the bpf_dynptr_read/write API 
and avoid needing the optval + len > optval_end check in the sleepable bpf prog. 
WDYT?

Regarding ->optlen, do you think the sleepable prog can stay with the 
bpf_dynptr_size() and bpf_dynptr_adjust() such that no need to expose optlen to 
the sleepable prog also.

> +		else
> +			info->reg_type = PTR_TO_PACKET;
>   		break;
>   	case offsetof(struct bpf_sockopt, optval_end):
>   		if (size != sizeof(__u64))
>   			return false;
> -		info->reg_type = PTR_TO_PACKET_END;
> +		if (prog->aux->sleepable)
> +			/* Prohibit access to the memory pointed by
> +			 * optval_end in sleepable programs.
> +			 */
> +			info->reg_type = PTR_TO_PACKET_END | MEM_USER;
> +		else
> +			info->reg_type = PTR_TO_PACKET_END;
>   		break;