| Message ID | 135feeafe6fe8d412e90865622e9601403c42be5.1708253445.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Take return from set_memory_ro() into account with bpf_prog_lock_ro() | expand |
On Sun, Feb 18, 2024 at 11:55:01AM +0100, Christophe Leroy wrote: > set_memory_ro() can fail, leaving memory unprotected. > > Check its return and take it into account as an error. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/filter.h | 5 +++-- > kernel/bpf/core.c | 4 +++- > kernel/bpf/verifier.c | 4 +++- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index fee070b9826e..fc0994dc5c72 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default) > > #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0])) > > -static inline void bpf_prog_lock_ro(struct bpf_prog *fp) > +static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp) > { > #ifndef CONFIG_BPF_JIT_ALWAYS_ON > if (!fp->jited) { > set_vm_flush_reset_perms(fp); > - set_memory_ro((unsigned long)fp, fp->pages); > + return set_memory_ro((unsigned long)fp, fp->pages); > } > #endif > + return 0; > } > > static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 71c459a51d9e..c49619ef55d0 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err) > } > > finalize: > - bpf_prog_lock_ro(fp); > + *err = bpf_prog_lock_ro(fp); > + if (*err) > + return fp; Weird error path, but yes. > > /* The tail call compatibility check can only be done at > * this late stage as we need to determine, if we deal > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c5d68a9d8acc..1f831a6b4bbc 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env) > * bpf_prog_load will add the kallsyms for the main program. > */ > for (i = 1; i < env->subprog_cnt; i++) { > - bpf_prog_lock_ro(func[i]); > + err = bpf_prog_lock_ro(func[i]); > + if (err) > + goto out_free; > bpf_prog_kallsyms_add(func[i]); > } Just to double-check if memory permissions being correctly restored on this error path, I walked back through it and see that it ultimately lands on vfree(), which appears to just throw the entire mapping away, so I think that's safe. :) Reviewed-by: Kees Cook <keescook@chromium.org>
Hello Christophe, On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > set_memory_ro() can fail, leaving memory unprotected. > > Check its return and take it into account as an error. > I don't see a cover letter for this series, could you describe how set_memory_ro() could fail. (Most callsites of set_memory_ro() didn't check the return values) And maybe craft a selftest to check the expected return values. > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/filter.h | 5 +++-- > kernel/bpf/core.c | 4 +++- > kernel/bpf/verifier.c | 4 +++- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index fee070b9826e..fc0994dc5c72 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default) > > #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0])) > > -static inline void bpf_prog_lock_ro(struct bpf_prog *fp) > +static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp) > { > #ifndef CONFIG_BPF_JIT_ALWAYS_ON > if (!fp->jited) { > set_vm_flush_reset_perms(fp); > - set_memory_ro((unsigned long)fp, fp->pages); > + return set_memory_ro((unsigned long)fp, fp->pages); > } > #endif > + return 0; > } > > static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 71c459a51d9e..c49619ef55d0 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err) > } > > finalize: > - bpf_prog_lock_ro(fp); > + *err = bpf_prog_lock_ro(fp); > + if (*err) > + return fp; > > /* The tail call compatibility check can only be done at > * this late stage as we need to determine, if we deal > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c5d68a9d8acc..1f831a6b4bbc 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env) > * bpf_prog_load will add the kallsyms for the main program. > */ > for (i = 1; i < env->subprog_cnt; i++) { > - bpf_prog_lock_ro(func[i]); > + err = bpf_prog_lock_ro(func[i]); > + if (err) > + goto out_free; > bpf_prog_kallsyms_add(func[i]); > } > > -- > 2.43.0 > >
Le 19/02/2024 à 02:40, Hengqi Chen a écrit : > [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > Hello Christophe, > > On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> set_memory_ro() can fail, leaving memory unprotected. >> >> Check its return and take it into account as an error. >> > > I don't see a cover letter for this series, could you describe how > set_memory_ro() could fail. > (Most callsites of set_memory_ro() didn't check the return values) Yeah, there is no cover letter because as explained in patch 2 the two patches are autonomous. The only reason why I sent it as a series is because the patches both modify include/linux/filter.h in two places that are too close to each other. I should have added a link to https://github.com/KSPP/linux/issues/7 See that link for detailed explanation. If we take powerpc as an exemple, set_memory_ro() is a frontend to change_memory_attr(). When you look at change_memory_attr() you see it can return -EINVAL in two cases. Then it calls apply_to_existing_page_range(). When you go down the road you see you can get -EINVAL or -ENOMEM from that function or its callees. Christophe
On 2/19/24 7:39 AM, Christophe Leroy wrote: > > > Le 19/02/2024 à 02:40, Hengqi Chen a écrit : >> [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >> >> Hello Christophe, >> >> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy >> <christophe.leroy@csgroup.eu> wrote: >>> >>> set_memory_ro() can fail, leaving memory unprotected. >>> >>> Check its return and take it into account as an error. >>> >> >> I don't see a cover letter for this series, could you describe how >> set_memory_ro() could fail. >> (Most callsites of set_memory_ro() didn't check the return values) > > Yeah, there is no cover letter because as explained in patch 2 the two > patches are autonomous. The only reason why I sent it as a series is > because the patches both modify include/linux/filter.h in two places > that are too close to each other. > > I should have added a link to https://github.com/KSPP/linux/issues/7 > See that link for detailed explanation. > > If we take powerpc as an exemple, set_memory_ro() is a frontend to > change_memory_attr(). When you look at change_memory_attr() you see it > can return -EINVAL in two cases. Then it calls > apply_to_existing_page_range(). When you go down the road you see you > can get -EINVAL or -ENOMEM from that function or its callees. By that logic, don't you have the same issue when undoing all of this? E.g. take arch_protect_bpf_trampoline() / arch_unprotect_bpf_trampoline() which is not covered in here, but what happens if you set it first to ro and later the setting back to rw fails? How would the error path there look like? It's something you cannot recover. Thanks, Daniel
Le 21/02/2024 à 18:30, Daniel Borkmann a écrit : > On 2/19/24 7:39 AM, Christophe Leroy wrote: >> >> >> Le 19/02/2024 à 02:40, Hengqi Chen a écrit : >>> [Vous ne recevez pas souvent de courriers de hengqi.chen@gmail.com. >>> Découvrez pourquoi ceci est important à >>> https://aka.ms/LearnAboutSenderIdentification ] >>> >>> Hello Christophe, >>> >>> On Sun, Feb 18, 2024 at 6:55 PM Christophe Leroy >>> <christophe.leroy@csgroup.eu> wrote: >>>> >>>> set_memory_ro() can fail, leaving memory unprotected. >>>> >>>> Check its return and take it into account as an error. >>>> >>> >>> I don't see a cover letter for this series, could you describe how >>> set_memory_ro() could fail. >>> (Most callsites of set_memory_ro() didn't check the return values) >> >> Yeah, there is no cover letter because as explained in patch 2 the two >> patches are autonomous. The only reason why I sent it as a series is >> because the patches both modify include/linux/filter.h in two places >> that are too close to each other. >> >> I should have added a link to https://github.com/KSPP/linux/issues/7 >> See that link for detailed explanation. >> >> If we take powerpc as an exemple, set_memory_ro() is a frontend to >> change_memory_attr(). When you look at change_memory_attr() you see it >> can return -EINVAL in two cases. Then it calls >> apply_to_existing_page_range(). When you go down the road you see you >> can get -EINVAL or -ENOMEM from that function or its callees. > > By that logic, don't you have the same issue when undoing all of this? > E.g. take arch_protect_bpf_trampoline() / arch_unprotect_bpf_trampoline() > which is not covered in here, but what happens if you set it first to ro > and later the setting back to rw fails? How would the error path there > look like? It's something you cannot recover. > arch_protect_bpf_trampoline() is handled there https://patchwork.kernel.org/project/netdevbpf/patch/883c5a268483a89ab13ed630210328a926f16e5b.1708526584.git.christophe.leroy@csgroup.eu/ In case setting back to RW fails there is not security issue, the things will likely blow up later with a write access to write protected memory but in terms of security that's not a problem.
diff --git a/include/linux/filter.h b/include/linux/filter.h index fee070b9826e..fc0994dc5c72 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -881,14 +881,15 @@ bpf_ctx_narrow_access_offset(u32 off, u32 size, u32 size_default) #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0])) -static inline void bpf_prog_lock_ro(struct bpf_prog *fp) +static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp) { #ifndef CONFIG_BPF_JIT_ALWAYS_ON if (!fp->jited) { set_vm_flush_reset_perms(fp); - set_memory_ro((unsigned long)fp, fp->pages); + return set_memory_ro((unsigned long)fp, fp->pages); } #endif + return 0; } static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 71c459a51d9e..c49619ef55d0 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -2392,7 +2392,9 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err) } finalize: - bpf_prog_lock_ro(fp); + *err = bpf_prog_lock_ro(fp); + if (*err) + return fp; /* The tail call compatibility check can only be done at * this late stage as we need to determine, if we deal diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c5d68a9d8acc..1f831a6b4bbc 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19020,7 +19020,9 @@ static int jit_subprogs(struct bpf_verifier_env *env) * bpf_prog_load will add the kallsyms for the main program. */ for (i = 1; i < env->subprog_cnt; i++) { - bpf_prog_lock_ro(func[i]); + err = bpf_prog_lock_ro(func[i]); + if (err) + goto out_free; bpf_prog_kallsyms_add(func[i]); }
set_memory_ro() can fail, leaving memory unprotected. Check its return and take it into account as an error. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- include/linux/filter.h | 5 +++-- kernel/bpf/core.c | 4 +++- kernel/bpf/verifier.c | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-)