Re: [PATCH v2 6.1] bpf: Prevent tail call between progs attached to different hooks

[Greg KH <greg@kroah.com>]

On Tue, Jan 21, 2025 at 04:41:17PM +0100, hsimeliere.opensource@witekio.com wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> [ Upstream commit 28ead3eaabc16ecc907cfb71876da028080f6356 ]
> 
> bpf progs can be attached to kernel functions, and the attached functions
> can take different parameters or return different return values. If
> prog attached to one kernel function tail calls prog attached to another
> kernel function, the ctx access or return value verification could be
> bypassed.
> 
> For example, if prog1 is attached to func1 which takes only 1 parameter
> and prog2 is attached to func2 which takes two parameters. Since verifier
> assumes the bpf ctx passed to prog2 is constructed based on func2's
> prototype, verifier allows prog2 to access the second parameter from
> the bpf ctx passed to it. The problem is that verifier does not prevent
> prog1 from passing its bpf ctx to prog2 via tail call. In this case,
> the bpf ctx passed to prog2 is constructed from func1 instead of func2,
> that is, the assumption for ctx access verification is bypassed.
> 
> Another example, if BPF LSM prog1 is attached to hook file_alloc_security,
> and BPF LSM prog2 is attached to hook bpf_lsm_audit_rule_known. Verifier
> knows the return value rules for these two hooks, e.g. it is legal for
> bpf_lsm_audit_rule_known to return positive number 1, and it is illegal
> for file_alloc_security to return positive number. So verifier allows
> prog2 to return positive number 1, but does not allow prog1 to return
> positive number. The problem is that verifier does not prevent prog1
> from calling prog2 via tail call. In this case, prog2's return value 1
> will be used as the return value for prog1's hook file_alloc_security.
> That is, the return value rule is bypassed.
> 
> This patch adds restriction for tail call to prevent such bypasses.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> Link: https://lore.kernel.org/r/20240719110059.797546-4-xukuohai@huaweicloud.com
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> [ Deletion of the patch line on the condition using bpf_prog_is_dev_bound as
>  it was added by commit 3d76a4d3d4e591af3e789698affaad88a5a8e8ab which is not
>  present in version 6.1 ]
> Signed-off-by: BRUNO VERNAY <bruno.vernay@se.com>
> Signed-off-by: Hugo SIMELIERE <hsimeliere.opensource@witekio.com>
> ---
>  include/linux/bpf.h |  1 +
>  kernel/bpf/core.c   | 19 +++++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)

Why is this commit needed in the 6.1.y kernel tree?

confused,

greg k-h