| Message ID | 20240906061449.734300-1-koichiro.den@canonical.com |
|---|---|
| Headers | show |
| Series | CVE-2024-26885 | expand |
On Fri, Sep 06, 2024 at 03:14:37PM +0900, Koichiro Den wrote: > [Impact] > > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > The devmap code allocates a number hash buckets equal to the next power > of two of the max_entries value provided when creating the map. When > rounding up to the next power of two, the 32-bit variable storing the > number of buckets can overflow, and the code checks for overflow by > checking if the truncated 32-bit value is equal to 0. However, on 32-bit > arches the rounding up itself can overflow mid-way through, because it > ends up doing a left-shift of 32 bits on an unsigned long value. If the > size of an unsigned long is four bytes, this is undefined behaviour, so > there is no guarantee that we'll end up with a nice and tidy 0-value at > the end. > > Syzbot managed to turn this into a crash on arm32 by creating a > DEVMAP_HASH with max_entries > 0x80000000 and then trying to update it. > Fix this by moving the overflow check to before the rounding up > operation. > > [Backport] > > There were conflicts due to missing commits: > - commit 844f157f6c0a ("bpf: Eliminate rlimit-based memory accounting for devmap maps") > - commit 96360004b862 ("xdp: Make devmap flush_list common for all map instances") > > and resolved these conflicts by adjusting contexts without merging them > to avoid unnecessary changes to the code base. > > [Fix] > > Noble: not affected > Jammy: fixed via stable > Focal: Backport - adjusted contexts due to missing commits, see [Backport] > Bionic: not affected > Xenial: not affected > Trusty: not affected > > [Test Case] > > Compile and boot tested. > > Also (stress) tested using the syzbot reproducer [1] on my local armhf > focal qemu instance, with and without this backport, though it did not > trigger the reported issue even without the backport, likely due to a > compiler difference from syzbot (i.e. the original overflow checking > seemed to work even without the backport). I've not investigated more > into it, but still the test confirms the robustness of this backport. > > [1] https://syzkaller.appspot.com/bug?extid=8cd36f6b65f3cafd400a > > [Where problems could occur] > > This fix potentially affects 32-bit arches, an issue with this fix would > cause underfined behaviour when creating a DEVMAP_HASH with max_entries > greater than 0x80000000. > > > Toke Høiland-Jørgensen (1): > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > kernel/bpf/devmap.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > -- > 2.43.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Acked-by: Manuel Diewald <manuel.diewald@canonical.com>
Acked-by: Thibault Ferrante <thibault.ferrante@canonical.com> On 06-09-2024 08:14, Koichiro Den wrote: > [Impact] > > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > The devmap code allocates a number hash buckets equal to the next power > of two of the max_entries value provided when creating the map. When > rounding up to the next power of two, the 32-bit variable storing the > number of buckets can overflow, and the code checks for overflow by > checking if the truncated 32-bit value is equal to 0. However, on 32-bit > arches the rounding up itself can overflow mid-way through, because it > ends up doing a left-shift of 32 bits on an unsigned long value. If the > size of an unsigned long is four bytes, this is undefined behaviour, so > there is no guarantee that we'll end up with a nice and tidy 0-value at > the end. > > Syzbot managed to turn this into a crash on arm32 by creating a > DEVMAP_HASH with max_entries > 0x80000000 and then trying to update it. > Fix this by moving the overflow check to before the rounding up > operation. > > [Backport] > > There were conflicts due to missing commits: > - commit 844f157f6c0a ("bpf: Eliminate rlimit-based memory accounting for devmap maps") > - commit 96360004b862 ("xdp: Make devmap flush_list common for all map instances") > > and resolved these conflicts by adjusting contexts without merging them > to avoid unnecessary changes to the code base. > > [Fix] > > Noble: not affected > Jammy: fixed via stable > Focal: Backport - adjusted contexts due to missing commits, see [Backport] > Bionic: not affected > Xenial: not affected > Trusty: not affected > > [Test Case] > > Compile and boot tested. > > Also (stress) tested using the syzbot reproducer [1] on my local armhf > focal qemu instance, with and without this backport, though it did not > trigger the reported issue even without the backport, likely due to a > compiler difference from syzbot (i.e. the original overflow checking > seemed to work even without the backport). I've not investigated more > into it, but still the test confirms the robustness of this backport. > > [1] https://syzkaller.appspot.com/bug?extid=8cd36f6b65f3cafd400a > > [Where problems could occur] > > This fix potentially affects 32-bit arches, an issue with this fix would > cause underfined behaviour when creating a DEVMAP_HASH with max_entries > greater than 0x80000000. > > > Toke Høiland-Jørgensen (1): > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > kernel/bpf/devmap.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) >
On 06.09.24 08:14, Koichiro Den wrote: > [Impact] > > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > The devmap code allocates a number hash buckets equal to the next power > of two of the max_entries value provided when creating the map. When > rounding up to the next power of two, the 32-bit variable storing the > number of buckets can overflow, and the code checks for overflow by > checking if the truncated 32-bit value is equal to 0. However, on 32-bit > arches the rounding up itself can overflow mid-way through, because it > ends up doing a left-shift of 32 bits on an unsigned long value. If the > size of an unsigned long is four bytes, this is undefined behaviour, so > there is no guarantee that we'll end up with a nice and tidy 0-value at > the end. > > Syzbot managed to turn this into a crash on arm32 by creating a > DEVMAP_HASH with max_entries > 0x80000000 and then trying to update it. > Fix this by moving the overflow check to before the rounding up > operation. > > [Backport] > > There were conflicts due to missing commits: > - commit 844f157f6c0a ("bpf: Eliminate rlimit-based memory accounting for devmap maps") > - commit 96360004b862 ("xdp: Make devmap flush_list common for all map instances") > > and resolved these conflicts by adjusting contexts without merging them > to avoid unnecessary changes to the code base. > > [Fix] > > Noble: not affected > Jammy: fixed via stable > Focal: Backport - adjusted contexts due to missing commits, see [Backport] > Bionic: not affected > Xenial: not affected > Trusty: not affected > > [Test Case] > > Compile and boot tested. > > Also (stress) tested using the syzbot reproducer [1] on my local armhf > focal qemu instance, with and without this backport, though it did not > trigger the reported issue even without the backport, likely due to a > compiler difference from syzbot (i.e. the original overflow checking > seemed to work even without the backport). I've not investigated more > into it, but still the test confirms the robustness of this backport. > > [1] https://syzkaller.appspot.com/bug?extid=8cd36f6b65f3cafd400a > > [Where problems could occur] > > This fix potentially affects 32-bit arches, an issue with this fix would > cause underfined behaviour when creating a DEVMAP_HASH with max_entries > greater than 0x80000000. > > > Toke Høiland-Jørgensen (1): > bpf: Fix DEVMAP_HASH overflow check on 32-bit arches > > kernel/bpf/devmap.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Applied to focal:linux/master-next. Thanks. -Stefan
[Impact] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches The devmap code allocates a number hash buckets equal to the next power of two of the max_entries value provided when creating the map. When rounding up to the next power of two, the 32-bit variable storing the number of buckets can overflow, and the code checks for overflow by checking if the truncated 32-bit value is equal to 0. However, on 32-bit arches the rounding up itself can overflow mid-way through, because it ends up doing a left-shift of 32 bits on an unsigned long value. If the size of an unsigned long is four bytes, this is undefined behaviour, so there is no guarantee that we'll end up with a nice and tidy 0-value at the end. Syzbot managed to turn this into a crash on arm32 by creating a DEVMAP_HASH with max_entries > 0x80000000 and then trying to update it. Fix this by moving the overflow check to before the rounding up operation. [Backport] There were conflicts due to missing commits: - commit 844f157f6c0a ("bpf: Eliminate rlimit-based memory accounting for devmap maps") - commit 96360004b862 ("xdp: Make devmap flush_list common for all map instances") and resolved these conflicts by adjusting contexts without merging them to avoid unnecessary changes to the code base. [Fix] Noble: not affected Jammy: fixed via stable Focal: Backport - adjusted contexts due to missing commits, see [Backport] Bionic: not affected Xenial: not affected Trusty: not affected [Test Case] Compile and boot tested. Also (stress) tested using the syzbot reproducer [1] on my local armhf focal qemu instance, with and without this backport, though it did not trigger the reported issue even without the backport, likely due to a compiler difference from syzbot (i.e. the original overflow checking seemed to work even without the backport). I've not investigated more into it, but still the test confirms the robustness of this backport. [1] https://syzkaller.appspot.com/bug?extid=8cd36f6b65f3cafd400a [Where problems could occur] This fix potentially affects 32-bit arches, an issue with this fix would cause underfined behaviour when creating a DEVMAP_HASH with max_entries greater than 0x80000000. Toke Høiland-Jørgensen (1): bpf: Fix DEVMAP_HASH overflow check on 32-bit arches kernel/bpf/devmap.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)