| Message ID | 20240123002814.1396804-32-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
On Tue, Jan 23, 2024 at 2:42 AM Kees Cook <keescook@chromium.org> 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@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: kvm@vger.kernel.org > Cc: virtualization@lists.linux.dev > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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@redhat.com> > + > 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 > >
On Fri, Jan 26, 2024 at 08:31:04PM +0100, Eugenio Perez Martin wrote: > On Tue, Jan 23, 2024 at 2:42 AM Kees Cook <keescook@chromium.org> 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@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Cc: kvm@vger.kernel.org > > Cc: virtualization@lists.linux.dev > > Cc: netdev@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > 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@redhat.com> Thanks! Yeah, you are not alone in suggesting "end" in a several of these patches. :) -Kees
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; + 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;
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@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Cc: kvm@vger.kernel.org Cc: virtualization@lists.linux.dev Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/vhost/vringh.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)