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

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

[hsimeliere.opensource]

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>
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(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7f4ce183dcb0..39291ec48374 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -250,6 +250,7 @@ struct bpf_map {
 	 * same prog type, JITed flag and xdp_has_frags flag.
 	 */
 	struct {
+		const struct btf_type *attach_func_proto;
 		spinlock_t lock;
 		enum bpf_prog_type type;
 		bool jited;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 83b416af4da1..c281f5b8705e 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2121,6 +2121,7 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(fp);
 	bool ret;
+	struct bpf_prog_aux *aux = fp->aux;
 
 	if (fp->kprobe_override)
 		return false;
@@ -2132,12 +2133,26 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
 		 */
 		map->owner.type  = prog_type;
 		map->owner.jited = fp->jited;
-		map->owner.xdp_has_frags = fp->aux->xdp_has_frags;
+		map->owner.xdp_has_frags = aux->xdp_has_frags;
+		map->owner.attach_func_proto = aux->attach_func_proto;
 		ret = true;
 	} else {
 		ret = map->owner.type  == prog_type &&
 		      map->owner.jited == fp->jited &&
-		      map->owner.xdp_has_frags == fp->aux->xdp_has_frags;
+		      map->owner.xdp_has_frags == aux->xdp_has_frags;
+		if (ret &&
+		    map->owner.attach_func_proto != aux->attach_func_proto) {
+			switch (prog_type) {
+			case BPF_PROG_TYPE_TRACING:
+			case BPF_PROG_TYPE_LSM:
+			case BPF_PROG_TYPE_EXT:
+			case BPF_PROG_TYPE_STRUCT_OPS:
+				ret = false;
+				break;
+			default:
+				break;
+			}
+		}
 	}
 	spin_unlock(&map->owner.lock);
 
-- 
2.43.0

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

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 28ead3eaabc16ecc907cfb71876da028080f6356

WARNING: Author mismatch between patch and upstream commit:
Backport author: hsimeliere.opensource@witekio.com
Commit author: Xu Kuohai<xukuohai@huawei.com>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.6.y | Present (different SHA1: 5d5e3b4cbe8e)
6.1.y | Not found

Note: The patch differs from the upstream commit:
---
1:  28ead3eaabc1 ! 1:  40e6bff2b282 bpf: Prevent tail call between progs attached to different hooks
    @@ Metadata
      ## Commit message ##
         bpf: Prevent tail call between progs attached to different hooks
     
    +    [ 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
    @@ Commit message
         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>
    +    Signed-off-by: BRUNO VERNAY <bruno.vernay@se.com>
    +    Signed-off-by: Hugo SIMELIERE <hsimeliere.opensource@witekio.com>
     
      ## include/linux/bpf.h ##
     @@ include/linux/bpf.h: struct bpf_map {
    @@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
      
      	if (fp->kprobe_override)
      		return false;
    -@@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
    - 	 * in the case of devmap and cpumap). Until device checks
    - 	 * are implemented, prohibit adding dev-bound programs to program maps.
    - 	 */
    --	if (bpf_prog_is_dev_bound(fp->aux))
    -+	if (bpf_prog_is_dev_bound(aux))
    - 		return false;
    - 
    - 	spin_lock(&map->owner.lock);
     @@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
      		 */
      		map->owner.type  = prog_type;
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Success   |

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

[Greg KH]

On Fri, Jan 10, 2025 at 09:40:00AM +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>
> Signed-off-by: BRUNO VERNAY <bruno.vernay@se.com>
> Signed-off-by: Hugo SIMELIERE <hsimeliere.opensource@witekio.com>

Please document what you are doing here that is needed for the backport
as this does NOT match up with what is upstream (a chunk is missing...)

thanks,

greg k-h

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

[hsimeliere.opensource]

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(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2189c0d18fa7..e9c1338851e3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -250,6 +250,7 @@ struct bpf_map {
 	 * same prog type, JITed flag and xdp_has_frags flag.
 	 */
 	struct {
+		const struct btf_type *attach_func_proto;
 		spinlock_t lock;
 		enum bpf_prog_type type;
 		bool jited;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 83b416af4da1..c281f5b8705e 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2121,6 +2121,7 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(fp);
 	bool ret;
+	struct bpf_prog_aux *aux = fp->aux;
 
 	if (fp->kprobe_override)
 		return false;
@@ -2132,12 +2133,26 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
 		 */
 		map->owner.type  = prog_type;
 		map->owner.jited = fp->jited;
-		map->owner.xdp_has_frags = fp->aux->xdp_has_frags;
+		map->owner.xdp_has_frags = aux->xdp_has_frags;
+		map->owner.attach_func_proto = aux->attach_func_proto;
 		ret = true;
 	} else {
 		ret = map->owner.type  == prog_type &&
 		      map->owner.jited == fp->jited &&
-		      map->owner.xdp_has_frags == fp->aux->xdp_has_frags;
+		      map->owner.xdp_has_frags == aux->xdp_has_frags;
+		if (ret &&
+		    map->owner.attach_func_proto != aux->attach_func_proto) {
+			switch (prog_type) {
+			case BPF_PROG_TYPE_TRACING:
+			case BPF_PROG_TYPE_LSM:
+			case BPF_PROG_TYPE_EXT:
+			case BPF_PROG_TYPE_STRUCT_OPS:
+				ret = false;
+				break;
+			default:
+				break;
+			}
+		}
 	}
 	spin_unlock(&map->owner.lock);
 
-- 
2.43.0

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

[Greg KH]

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

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

[hsimeliere.opensource]

This patch is needed to correct CVE-2024-50063
https://nvd.nist.gov/vuln/detail/CVE-2024-50063

Kind regards,
Hugo Simeliere

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

[Greg KH]

On Mon, Feb 10, 2025 at 10:44:07AM +0100, hsimeliere.opensource@witekio.com wrote:
> This patch is needed to correct CVE-2024-50063

What patch?  I see no context here :(

> https://nvd.nist.gov/vuln/detail/CVE-2024-50063

Never link to nvd, their "enhancements" are provably wrong and hurtful
to the kernel ecosystem.  Always just refer to cve.org records or better
yet, our own announcements.

That being said, why do you feel this commit is really needed in those
older kernels other than just trying to meet a "check box" requirement
somewhere?

thanks,

greg k-h

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

[hsimeliere.opensource]

Thank you for this information, I will take note of it for our next contribution. Does this also apply to more recent kernel versions? 

So the CVE must be under a CNA or CISA score for the patch to be required by the kernel?  
Where can I find your own announcements? 

Thanks,
Hugo Simeliere

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

[Greg KH]

On Mon, Feb 10, 2025 at 03:29:05PM +0100, hsimeliere.opensource@witekio.com wrote:
> Thank you for this information, I will take note of it for our next
> contribution. Does this also apply to more recent kernel versions? 

What information?  Sorry, but I see no context here at all, have you
read Documentation/process/email-clients.rst

Remember, some of us get 1000+ emails a day to deal with, context
matters.

thanks,

greg k-h

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

[hsimeliere.opensource]

On Mon, Feb 10, 2025 at 10:55:07AM +0100, gregkh@linuxfoundation.org wrote:

> Never link to nvd, their "enhancements" are provably wrong and hurtful
> to the kernel ecosystem.  Always just refer to cve.org records or better
> yet, our own announcements.

Thank you for this information, I will take note of it for our next contribution.
So the CVE must be under a CNA or CISA score for the patch to be required by the kernel?  
Where can I find your own announcements? 

Thanks,
Hugo Simeliere

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

[Greg KH]

On Mon, Feb 10, 2025 at 05:32:33PM +0100, hsimeliere.opensource@witekio.com wrote:
> On Mon, Feb 10, 2025 at 10:55:07AM +0100, gregkh@linuxfoundation.org wrote:
> 
> > Never link to nvd, their "enhancements" are provably wrong and hurtful
> > to the kernel ecosystem.  Always just refer to cve.org records or better
> > yet, our own announcements.
> 
> Thank you for this information, I will take note of it for our next contribution.
> So the CVE must be under a CNA or CISA score for the patch to be required by the kernel?  

The kernel CNA provides NO "score" as that obviously is impossible to do
given that we do NOT know your use case.

What exactly are you trying to do here?  Backport random changes to
older kernels for what reason?  We are glad to take backports for fixes
that did not apply to older kernels, but you have to test them and
provide a reason for why they should be included.  To not have that on
your side already feels very odd.

> Where can I find your own announcements? 

You have read the in-kernel documentation about how we handle CVEs,
right?  It's listed there :)

thanks,

greg k-h

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

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 28ead3eaabc16ecc907cfb71876da028080f6356

WARNING: Author mismatch between patch and upstream commit:
Backport author: hsimeliere.opensource@witekio.com
Commit author: Xu Kuohai<xukuohai@huawei.com>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.6.y | Present (different SHA1: 5d5e3b4cbe8e)
6.1.y | Not found

Note: The patch differs from the upstream commit:
---
1:  28ead3eaabc16 ! 1:  d2cd989f36650 bpf: Prevent tail call between progs attached to different hooks
    @@ Metadata
      ## Commit message ##
         bpf: Prevent tail call between progs attached to different hooks
     
    +    [ 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
    @@ Commit message
         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 ##
     @@ include/linux/bpf.h: struct bpf_map {
    @@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
      
      	if (fp->kprobe_override)
      		return false;
    -@@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
    - 	 * in the case of devmap and cpumap). Until device checks
    - 	 * are implemented, prohibit adding dev-bound programs to program maps.
    - 	 */
    --	if (bpf_prog_is_dev_bound(fp->aux))
    -+	if (bpf_prog_is_dev_bound(aux))
    - 		return false;
    - 
    - 	spin_lock(&map->owner.lock);
     @@ kernel/bpf/core.c: bool bpf_prog_map_compatible(struct bpf_map *map,
      		 */
      		map->owner.type  = prog_type;
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Success   |