* [PATCH 32/82] vringh: Refactor intentional wrap-around calculation [not found] <20240122235208.work.748-kees@kernel.org> @ 2024-01-23 0:27 ` Kees Cook 2024-01-26 19:31 ` Eugenio Perez Martin 2024-01-23 0:27 ` [PATCH 77/82] virtio: Refactor intentional wrap-around test Kees Cook 1 sibling, 1 reply; 5+ messages in thread From: Kees Cook @ 2024-01-23 0:27 UTC (permalink / raw) To: linux-hardening Cc: Kees Cook, Michael S. Tsirkin, Jason Wang, kvm, virtualization, netdev, Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel 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; + 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 32/82] vringh: Refactor intentional wrap-around calculation 2024-01-23 0:27 ` [PATCH 32/82] vringh: Refactor intentional wrap-around calculation Kees Cook @ 2024-01-26 19:31 ` Eugenio Perez Martin 2024-01-26 19:42 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Eugenio Perez Martin @ 2024-01-26 19:31 UTC (permalink / raw) To: Kees Cook Cc: linux-hardening, Michael S. Tsirkin, Jason Wang, kvm, virtualization, netdev, Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 32/82] vringh: Refactor intentional wrap-around calculation 2024-01-26 19:31 ` Eugenio Perez Martin @ 2024-01-26 19:42 ` Kees Cook 0 siblings, 0 replies; 5+ messages in thread From: Kees Cook @ 2024-01-26 19:42 UTC (permalink / raw) To: Eugenio Perez Martin Cc: linux-hardening, Michael S. Tsirkin, Jason Wang, kvm, virtualization, netdev, Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel 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 -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 77/82] virtio: Refactor intentional wrap-around test [not found] <20240122235208.work.748-kees@kernel.org> 2024-01-23 0:27 ` [PATCH 32/82] vringh: Refactor intentional wrap-around calculation Kees Cook @ 2024-01-23 0:27 ` Kees Cook 2024-01-26 19:33 ` Eugenio Perez Martin 1 sibling, 1 reply; 5+ messages in thread From: Kees Cook @ 2024-01-23 0:27 UTC (permalink / raw) To: linux-hardening Cc: Kees Cook, Michael S. Tsirkin, Jason Wang, Xuan Zhuo, virtualization, Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel 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 wrap-around addition test to use add_would_overflow(). This paves the way to enabling the wrap-around sanitizers 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: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Cc: virtualization@lists.linux.dev Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index 0d3dbfaf4b23..710d3bd45b4f 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -59,7 +59,7 @@ vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off, length -= start; - if (start + offset < offset) { + if (add_would_overflow(offset, start)) { dev_err(&dev->dev, "virtio_pci: map wrap-around %u+%u\n", start, offset); @@ -81,7 +81,7 @@ vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off, if (len) *len = length; - if (minlen + offset < minlen || + if (add_would_overflow(minlen, offset) || minlen + offset > pci_resource_len(dev, bar)) { dev_err(&dev->dev, "virtio_pci: map virtio %zu@%u " -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 77/82] virtio: Refactor intentional wrap-around test 2024-01-23 0:27 ` [PATCH 77/82] virtio: Refactor intentional wrap-around test Kees Cook @ 2024-01-26 19:33 ` Eugenio Perez Martin 0 siblings, 0 replies; 5+ messages in thread From: Eugenio Perez Martin @ 2024-01-26 19:33 UTC (permalink / raw) To: Kees Cook Cc: linux-hardening, Michael S. Tsirkin, Jason Wang, Xuan Zhuo, virtualization, Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel On Tue, Jan 23, 2024 at 2:28 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 wrap-around addition test to use add_would_overflow(). > This paves the way to enabling the wrap-around sanitizers 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: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > Cc: virtualization@lists.linux.dev > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Eugenio Pérez <eperezma@redhat.com> > --- > drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > index 0d3dbfaf4b23..710d3bd45b4f 100644 > --- a/drivers/virtio/virtio_pci_modern_dev.c > +++ b/drivers/virtio/virtio_pci_modern_dev.c > @@ -59,7 +59,7 @@ vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off, > > length -= start; > > - if (start + offset < offset) { > + if (add_would_overflow(offset, start)) { > dev_err(&dev->dev, > "virtio_pci: map wrap-around %u+%u\n", > start, offset); > @@ -81,7 +81,7 @@ vp_modern_map_capability(struct virtio_pci_modern_device *mdev, int off, > if (len) > *len = length; > > - if (minlen + offset < minlen || > + if (add_would_overflow(minlen, offset) || > minlen + offset > pci_resource_len(dev, bar)) { > dev_err(&dev->dev, > "virtio_pci: map virtio %zu@%u " > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-26 19:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240122235208.work.748-kees@kernel.org>
2024-01-23 0:27 ` [PATCH 32/82] vringh: Refactor intentional wrap-around calculation Kees Cook
2024-01-26 19:31 ` Eugenio Perez Martin
2024-01-26 19:42 ` Kees Cook
2024-01-23 0:27 ` [PATCH 77/82] virtio: Refactor intentional wrap-around test Kees Cook
2024-01-26 19:33 ` Eugenio Perez Martin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox