On Tue, Jan 23, 2024 at 2:42 AM Kees Cook <keescook@xxxxxxxxxxxx> 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 > > Notably, 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[2], unsigned[3], > or pointer[4] 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/26 [2] > Link: https://github.com/KSPP/linux/issues/27 [3] > Link: https://github.com/KSPP/linux/issues/344 [4] > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx> > Cc: Jason Wang <jasowang@xxxxxxxxxx> > Cc: kvm@xxxxxxxxxxxxxxx > Cc: virtualization@xxxxxxxxxxxxxxx > Cc: netdev@xxxxxxxxxxxxxxx > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > --- > drivers/vhost/vringh.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 7b8fd977f71c..07442f0a52bd 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -145,6 +145,8 @@ static inline bool range_check(struct vringh *vrh, u64 addr, size_t *len, > bool (*getrange)(struct vringh *, > u64, struct vringh_range *)) > { > + u64 sum; I understand this is part of a bulk change so little time to think about names :). But what about "end" or similar? Either way, Acked-by: Eugenio Pérez <eperezma@xxxxxxxxxx> > + > if (addr < range->start || addr > range->end_incl) { > if (!getrange(vrh, addr, range)) > return false; > @@ -152,20 +154,20 @@ static inline bool range_check(struct vringh *vrh, u64 addr, size_t *len, > BUG_ON(addr < range->start || addr > range->end_incl); > > /* To end of memory? */ > - if (unlikely(addr + *len == 0)) { > + if (unlikely(U64_MAX - addr == *len)) { > if (range->end_incl == -1ULL) > return true; > goto truncate; > } > > /* Otherwise, don't wrap. */ > - if (addr + *len < addr) { > + if (check_add_overflow(addr, *len, &sum)) { > vringh_bad("Wrapping descriptor %zu@0x%llx", > *len, (unsigned long long)addr); > return false; > } > > - if (unlikely(addr + *len - 1 > range->end_incl)) > + if (unlikely(sum - 1 > range->end_incl)) > goto truncate; > return true; > > -- > 2.34.1 > >
