Re: [PATCH v6 bpf-next 1/3] bpf: Add bpf_copy_from_user_task() helper

From: Kenny Yu <kennyyu@fb.com>
To: <yhs@fb.com>
Cc: <alexei.starovoitov@gmail.com>, <andrii.nakryiko@gmail.com>,
	<andrii@kernel.org>, <ast@kernel.org>, <bpf@vger.kernel.org>,
	<daniel@iogearbox.net>, <kennyyu@fb.com>, <phoenix1987@gmail.com>
Subject: Re: [PATCH v6 bpf-next 1/3] bpf: Add bpf_copy_from_user_task() helper
Date: Mon, 24 Jan 2022 09:30:27 -0800	[thread overview]
Message-ID: <20220124173027.1868912-1-kennyyu@fb.com> (raw)
In-Reply-To: <170be4e4-6709-cfae-f728-81fc4452f111@fb.com>

> "On error dst buffer is zeroed out."? This is an explicit guarantee.

Will add to the docs.

> is there a point in calling access_process_vm() with size == 0? It
> would validate that get_task_mm() succeeds, but that's pretty much it?
> So maybe instead just exit early if size is zero? It will be also less
> convoluted logic:
> 
> if (size == 0)
>     return 0;
> if (access_process_vm(...)) {
>     memset(0);
>     return -EFAULT;
> }
> return 0;

Will do an explicit check for `size == 0`.
Note that we still need to check if the return value of
`access_process_vm` == `size` to see if we have a partial read.

> > Without the above change, using bpf_copy_from_user_task() will trigger
> > rcu warning and may produce incorrect result. One option is to put
> > the above in a preparation patch before introducing
> > bpf_copy_from_user_task() so we won't have bisecting issues.
>
> Sure, patch #1 for sleepable bpf_iter, patch #2 for the helper? I
> mean, it's not a big deal, but both seem to deserve their own focused
> patches.

I'll split this into 2 patches and place the bpf_iter patch first.

> I appreciate that existing helpers already do this and it's good to
> follow suit for consistency, but what is the rationale behind zeroing
> memory on failure?

I believe the intent behind this is for security/privacy reasons.
On error, we don't want to unintentionally leak partially read data.

Thanks everyone for the suggestions!

Kenny