On 8/16/23 17:55, Martin KaFai Lau wrote:
On 8/15/23 10:47 AM, thinker.li@xxxxxxxxx wrote:
From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
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@xxxxxxxxx>
---
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?
It has been checked.
optval & optval_end are exported and can be used to compute the size
of the user space buffer. However, it can not be used to read the
content of the user space buffer.
To be specific, __check_mem_access() will fail due to having MEM_USER
in reg->type. However, it is implicit. I will make it explicit if
necessary.
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?
Then, we need to export another variable to get the size of the buffer
pointed by optval. Then, I would like to have a new context type
instead of struct bpf_sockopt for the sleepable programs. With the new
type, we can remove optval & optval_end completely from the view of
sleepable ones. They will get errors of accessing optval & optval_end
as early as compile time.
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;
