| Message ID | 20221111125620.754855-1-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1f6e04a1c7b85da3b765ca9f46029e5d1826d839 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf: Fix offset calculation error in __copy_map_value and zero_map_value | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf, async |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1362 this patch: 1362 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 157 this patch: 157 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1352 this patch: 1352 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-PR | success | PR summary |
| bpf/vmtest-bpf-VM_Test-5 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-VM_Test-3 | fail | Logs for build for aarch64 with gcc |
| bpf/vmtest-bpf-VM_Test-4 | fail | Logs for build for aarch64 with llvm-16 |
| bpf/vmtest-bpf-VM_Test-6 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-VM_Test-7 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-VM_Test-8 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-VM_Test-9 | success | Logs for set-matrix |
| bpf/vmtest-bpf-VM_Test-2 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-VM_Test-1 | success | Logs for ShellCheck |
On 11/11, Xu Kuohai wrote: > From: Xu Kuohai <xukuohai@huawei.com> > Function __copy_map_value and zero_map_value miscalculated copy offset, > resulting in possible copy of unwanted data to user or kernel. > Fix it. > Fixes: cc48755808c6 ("bpf: Add zero_map_value to zero map value with > special fields") > Fixes: 4d7d7f69f4b1 ("bpf: Adapt copy_map_value for multiple offset case") > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > include/linux/bpf.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 74c6f449d81e..c1bd1bd10506 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -315,7 +315,7 @@ static inline void __copy_map_value(struct bpf_map > *map, void *dst, void *src, b > u32 next_off = map->off_arr->field_off[i]; > memcpy(dst + curr_off, src + curr_off, next_off - curr_off); > - curr_off += map->off_arr->field_sz[i]; > + curr_off = next_off + map->off_arr->field_sz[i]; > } > memcpy(dst + curr_off, src + curr_off, map->value_size - curr_off); > } > @@ -344,7 +344,7 @@ static inline void zero_map_value(struct bpf_map > *map, void *dst) > u32 next_off = map->off_arr->field_off[i]; > memset(dst + curr_off, 0, next_off - curr_off); > - curr_off += map->off_arr->field_sz[i]; > + curr_off = next_off + map->off_arr->field_sz[i]; > } > memset(dst + curr_off, 0, map->value_size - curr_off); > } Hmm, does it mean that it currently works only for the cases where these special fields are first/last? Also, what about bpf-next? The same problem seem to exist there? Might be a good idea to have some selftest to exercise this? > -- > 2.30.2
On Fri, Nov 11, 2022 at 06:26:20PM IST, Xu Kuohai wrote: > From: Xu Kuohai <xukuohai@huawei.com> > > Function __copy_map_value and zero_map_value miscalculated copy offset, > resulting in possible copy of unwanted data to user or kernel. > > Fix it. > > Fixes: cc48755808c6 ("bpf: Add zero_map_value to zero map value with special fields") > Fixes: 4d7d7f69f4b1 ("bpf: Adapt copy_map_value for multiple offset case") > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> I also have a fix here for bpf-next, since this won't apply to it: https://lore.kernel.org/bpf/20221111193224.876706-4-memxor@gmail.com I think it'd be best if this one gets applied to bpf and mine to bpf-next, and any conflicts are resolved when merging both trees (the conflict is trivial), but I'll leave it up to the maintainers to decide.
On Sat, Nov 12, 2022 at 12:47:52AM IST, sdf@google.com wrote: > On 11/11, Xu Kuohai wrote: > > From: Xu Kuohai <xukuohai@huawei.com> > > > Function __copy_map_value and zero_map_value miscalculated copy offset, > > resulting in possible copy of unwanted data to user or kernel. > > > Fix it. > > > Fixes: cc48755808c6 ("bpf: Add zero_map_value to zero map value with > > special fields") > > Fixes: 4d7d7f69f4b1 ("bpf: Adapt copy_map_value for multiple offset case") > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > > --- > > include/linux/bpf.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 74c6f449d81e..c1bd1bd10506 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -315,7 +315,7 @@ static inline void __copy_map_value(struct bpf_map > > *map, void *dst, void *src, b > > u32 next_off = map->off_arr->field_off[i]; > > > memcpy(dst + curr_off, src + curr_off, next_off - curr_off); > > - curr_off += map->off_arr->field_sz[i]; > > + curr_off = next_off + map->off_arr->field_sz[i]; > > } > > memcpy(dst + curr_off, src + curr_off, map->value_size - curr_off); > > } > > @@ -344,7 +344,7 @@ static inline void zero_map_value(struct bpf_map > > *map, void *dst) > > u32 next_off = map->off_arr->field_off[i]; > > > memset(dst + curr_off, 0, next_off - curr_off); > > - curr_off += map->off_arr->field_sz[i]; > > + curr_off = next_off + map->off_arr->field_sz[i]; > > } > > memset(dst + curr_off, 0, map->value_size - curr_off); > > } > > Hmm, does it mean that it currently works only for the cases where > these special fields are first/last? > > Also, what about bpf-next? The same problem seem to exist there? > Replied with the patch in the other email. > Might be a good idea to have some selftest to exercise this? > I agree, there was another bug in the same code before this, so I think we should add tests for this (I should have done that with the commit being fixed...). Xu, if you have cycles, can you work on testing a few edge cases and make sure we don't regress in the future? Otherwise I will take a look next week.
Hello: This patch was applied to bpf/bpf.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Fri, 11 Nov 2022 07:56:20 -0500 you wrote: > From: Xu Kuohai <xukuohai@huawei.com> > > Function __copy_map_value and zero_map_value miscalculated copy offset, > resulting in possible copy of unwanted data to user or kernel. > > Fix it. > > [...] Here is the summary with links: - [bpf] bpf: Fix offset calculation error in __copy_map_value and zero_map_value https://git.kernel.org/bpf/bpf/c/1f6e04a1c7b8 You are awesome, thank you!
On 11/12/2022 4:45 AM, Kumar Kartikeya Dwivedi wrote: > On Sat, Nov 12, 2022 at 12:47:52AM IST, sdf@google.com wrote: >> On 11/11, Xu Kuohai wrote: >>> From: Xu Kuohai <xukuohai@huawei.com> >> >>> Function __copy_map_value and zero_map_value miscalculated copy offset, >>> resulting in possible copy of unwanted data to user or kernel. >> >>> Fix it. >> >>> Fixes: cc48755808c6 ("bpf: Add zero_map_value to zero map value with >>> special fields") >>> Fixes: 4d7d7f69f4b1 ("bpf: Adapt copy_map_value for multiple offset case") >>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >>> --- >>> include/linux/bpf.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >>> index 74c6f449d81e..c1bd1bd10506 100644 >>> --- a/include/linux/bpf.h >>> +++ b/include/linux/bpf.h >>> @@ -315,7 +315,7 @@ static inline void __copy_map_value(struct bpf_map >>> *map, void *dst, void *src, b >>> u32 next_off = map->off_arr->field_off[i]; >> >>> memcpy(dst + curr_off, src + curr_off, next_off - curr_off); >>> - curr_off += map->off_arr->field_sz[i]; >>> + curr_off = next_off + map->off_arr->field_sz[i]; >>> } >>> memcpy(dst + curr_off, src + curr_off, map->value_size - curr_off); >>> } >>> @@ -344,7 +344,7 @@ static inline void zero_map_value(struct bpf_map >>> *map, void *dst) >>> u32 next_off = map->off_arr->field_off[i]; >> >>> memset(dst + curr_off, 0, next_off - curr_off); >>> - curr_off += map->off_arr->field_sz[i]; >>> + curr_off = next_off + map->off_arr->field_sz[i]; >>> } >>> memset(dst + curr_off, 0, map->value_size - curr_off); >>> } >> >> Hmm, does it mean that it currently works only for the cases where >> these special fields are first/last? >> >> Also, what about bpf-next? The same problem seem to exist there? >> > > Replied with the patch in the other email. > >> Might be a good idea to have some selftest to exercise this? >> > > I agree, there was another bug in the same code before this, so I think we > should add tests for this (I should have done that with the commit being > fixed...). > > Xu, if you have cycles, can you work on testing a few edge cases and make sure > we don't regress in the future? Otherwise I will take a look next week. > . Ok, I'll add a few cases to test_sk_storage_map to capture this
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 74c6f449d81e..c1bd1bd10506 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -315,7 +315,7 @@ static inline void __copy_map_value(struct bpf_map *map, void *dst, void *src, b u32 next_off = map->off_arr->field_off[i]; memcpy(dst + curr_off, src + curr_off, next_off - curr_off); - curr_off += map->off_arr->field_sz[i]; + curr_off = next_off + map->off_arr->field_sz[i]; } memcpy(dst + curr_off, src + curr_off, map->value_size - curr_off); } @@ -344,7 +344,7 @@ static inline void zero_map_value(struct bpf_map *map, void *dst) u32 next_off = map->off_arr->field_off[i]; memset(dst + curr_off, 0, next_off - curr_off); - curr_off += map->off_arr->field_sz[i]; + curr_off = next_off + map->off_arr->field_sz[i]; } memset(dst + curr_off, 0, map->value_size - curr_off); }