| Message ID | 20240123002814.1396804-23-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
On Mon, Jan 22, 2024, Kees Cook wrote: > In an effort to separate intentional arithmetic wrap-around from > unexpected wrap-around, we need to refactor places that depend on this > kind of math. One of the most common code patterns of this is: > > VAR + value < VAR > > Notable, this is considered "undefined behavior" for signed and pointer > types, which the kernel works around by using the -fno-strict-overflow > option in the build[1] (which used to just be -fwrapv). Regardless, we > want to get the kernel source to the position where we can meaningfully > instrument arithmetic wrap-around conditions and catch them when they > are unexpected, regardless of whether they are signed, unsigned, or > pointer types. > > Refactor open-coded unsigned wrap-around addition test to use > check_add_overflow(), retaining the result for later usage (which removes > the redundant open-coded addition). This paves the way to enabling the > unsigned wrap-around sanitizer[2] in the future. > > Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] > Link: https://github.com/KSPP/linux/issues/27 [2] > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: kvm@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > virt/kvm/coalesced_mmio.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 1b90acb6e3fe..0a3b706fbf4c 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -25,17 +25,19 @@ static inline struct kvm_coalesced_mmio_dev *to_mmio(struct kvm_io_device *dev) > static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, > gpa_t addr, int len) > { > + gpa_t sum; s/sum/end? Also, given that your're fixing a gpa_t, which is a u64, presumably that means that this code in __kvm_set_memory_region() also needs to be fixed: if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) return -EINVAL; and for that one I'd really like to avoid an ignored output parameter (KVM converts the incoming mem->memory_size to pages, so the "sum" is never used directly). Would it make sense to add an API that feeds a dummy "sum" value? I assume UBSAN won't fire on the usage of the known good value, i.e. using the output parameter isn't necessary for functional correctness. Having an API that does just the check would trim down the size of many of these patches and avoid having to come up with names for the local variables. And IMO, the existing code is a wee bit more intuitive, it'd be nice to give developers the flexibility to choose which flavor yields the "best" code on a case-by-case basis. > + > /* is it in a batchable area ? > * (addr,len) is fully included in > * (zone->addr, zone->size) > */ > if (len < 0) > return 0; > - if (addr + len < addr) > + if (check_add_overflow(addr, len, &sum)) > return 0; > if (addr < dev->zone.addr) > return 0; > - if (addr + len > dev->zone.addr + dev->zone.size) > + if (sum > dev->zone.addr + dev->zone.size) > return 0; > return 1; > } > -- > 2.34.1 >
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 1b90acb6e3fe..0a3b706fbf4c 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -25,17 +25,19 @@ static inline struct kvm_coalesced_mmio_dev *to_mmio(struct kvm_io_device *dev) static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, gpa_t addr, int len) { + gpa_t sum; + /* is it in a batchable area ? * (addr,len) is fully included in * (zone->addr, zone->size) */ if (len < 0) return 0; - if (addr + len < addr) + if (check_add_overflow(addr, len, &sum)) return 0; if (addr < dev->zone.addr) return 0; - if (addr + len > dev->zone.addr + dev->zone.size) + if (sum > dev->zone.addr + dev->zone.size) return 0; return 1; }
In an effort to separate intentional arithmetic wrap-around from unexpected wrap-around, we need to refactor places that depend on this kind of math. One of the most common code patterns of this is: VAR + value < VAR Notable, this is considered "undefined behavior" for signed and pointer types, which the kernel works around by using the -fno-strict-overflow option in the build[1] (which used to just be -fwrapv). Regardless, we want to get the kernel source to the position where we can meaningfully instrument arithmetic wrap-around conditions and catch them when they are unexpected, regardless of whether they are signed, unsigned, or pointer types. Refactor open-coded unsigned wrap-around addition test to use check_add_overflow(), retaining the result for later usage (which removes the redundant open-coded addition). This paves the way to enabling the unsigned wrap-around sanitizer[2] in the future. Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] Link: https://github.com/KSPP/linux/issues/27 [2] Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- virt/kvm/coalesced_mmio.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)