netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 12/82] ipv4: Silence intentional wrapping addition
       [not found] <20240122235208.work.748-kees@kernel.org>
@ 2024-01-23  0:26 ` Kees Cook
  2024-01-23  0:27 ` [PATCH 28/82] niu: Refactor intentional wrap-around calculation Kees Cook
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2024-01-23  0:26 UTC (permalink / raw)
  To: linux-hardening
  Cc: Kees Cook, Jakub Kicinski, David S. Miller, David Ahern,
	Eric Dumazet, Paolo Abeni, netdev, Gustavo A. R. Silva,
	Bill Wendling, Justin Stitt, linux-kernel

The overflow sanitizer quickly noticed what appears to have been an old
sore spot involving intended wrap around:

[   22.192362] ------------[ cut here ]------------
[   22.193329] UBSAN: signed-integer-overflow in ../arch/x86/include/asm/atomic.h:85:11
[   22.194844] 1469769800 + 1671667352 cannot be represented in type 'int'
[   22.195975] CPU: 2 PID: 2260 Comm: nmbd Not tainted 6.7.0 #1
[   22.196927] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
[   22.198231] Call Trace:
[   22.198641]  <TASK>
[   22.198641]  dump_stack_lvl+0x64/0x80
[   22.199533]  handle_overflow+0x152/0x1a0
[   22.200382]  __ip_select_ident+0xe3/0x100

Explicitly perform a wrapping addition to solve for the needed
-fno-strict-overflow behavior but still allow the sanitizers to operate
correctly.

To see the (unchanged) assembly results more clearly, see:
https://godbolt.org/z/EhYhz6zTT

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/ipv4/route.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 16615d107cf0..c52e85b06fe7 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -473,11 +473,11 @@ static u32 ip_idents_reserve(u32 hash, int segs)
 	if (old != now && cmpxchg(p_tstamp, old, now) == old)
 		delta = get_random_u32_below(now - old);
 
-	/* If UBSAN reports an error there, please make sure your compiler
-	 * supports -fno-strict-overflow before reporting it that was a bug
-	 * in UBSAN, and it has been fixed in GCC-8.
+	/* If UBSAN reports an error there, please make sure your arch's
+	 * atomic_add_return() implementation has been annotated with
+	 * __signed_wrap.
 	 */
-	return atomic_add_return(segs + delta, p_id) - segs;
+	return atomic_add_return(add_wrap(segs, delta), p_id) - segs;
 }
 
 void __ip_select_ident(struct net *net, struct iphdr *iph, int segs)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 28/82] niu: Refactor intentional wrap-around calculation
       [not found] <20240122235208.work.748-kees@kernel.org>
  2024-01-23  0:26 ` [PATCH 12/82] ipv4: Silence intentional wrapping addition Kees Cook
@ 2024-01-23  0:27 ` Kees Cook
  2024-01-23  0:27 ` [PATCH 29/82] rds: " Kees Cook
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2024-01-23  0:27 UTC (permalink / raw)
  To: linux-hardening
  Cc: Kees Cook, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Du Cheng, 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: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/sun/niu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 21431f43e4c2..a4de07c6e618 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -6877,15 +6877,16 @@ static int niu_get_eeprom(struct net_device *dev,
 {
 	struct niu *np = netdev_priv(dev);
 	u32 offset, len, val;
+	u32 sum;
 
 	offset = eeprom->offset;
 	len = eeprom->len;
 
-	if (offset + len < offset)
+	if (check_add_overflow(offset, len, &sum))
 		return -EINVAL;
 	if (offset >= np->eeprom_len)
 		return -EINVAL;
-	if (offset + len > np->eeprom_len)
+	if (sum > np->eeprom_len)
 		len = eeprom->len = np->eeprom_len - offset;
 
 	if (offset & 3) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 29/82] rds: Refactor intentional wrap-around calculation
       [not found] <20240122235208.work.748-kees@kernel.org>
  2024-01-23  0:26 ` [PATCH 12/82] ipv4: Silence intentional wrapping addition Kees Cook
  2024-01-23  0:27 ` [PATCH 28/82] niu: Refactor intentional wrap-around calculation Kees Cook
@ 2024-01-23  0:27 ` Kees Cook
  2024-01-23  0:27 ` [PATCH 32/82] vringh: " Kees Cook
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2024-01-23  0:27 UTC (permalink / raw)
  To: linux-hardening
  Cc: Kees Cook, Santosh Shilimkar, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, netdev, linux-rdma, rds-devel,
	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: Santosh Shilimkar <santosh.shilimkar@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: rds-devel@oss.oracle.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/rds/info.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/rds/info.c b/net/rds/info.c
index b6b46a8214a0..87b35d07ce04 100644
--- a/net/rds/info.c
+++ b/net/rds/info.c
@@ -163,6 +163,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval,
 	unsigned long nr_pages = 0;
 	unsigned long start;
 	rds_info_func func;
+	unsigned long sum;
 	struct page **pages = NULL;
 	int ret;
 	int len;
@@ -175,7 +176,8 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval,
 
 	/* check for all kinds of wrapping and the like */
 	start = (unsigned long)optval;
-	if (len < 0 || len > INT_MAX - PAGE_SIZE + 1 || start + len < start) {
+	if (len < 0 || len > INT_MAX - PAGE_SIZE + 1 ||
+	    check_add_overflow(start, len, &sum)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -184,7 +186,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval,
 	if (len == 0)
 		goto call_func;
 
-	nr_pages = (PAGE_ALIGN(start + len) - (start & PAGE_MASK))
+	nr_pages = (PAGE_ALIGN(sum) - (start & PAGE_MASK))
 			>> PAGE_SHIFT;
 
 	pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_KERNEL);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 32/82] vringh: Refactor intentional wrap-around calculation
       [not found] <20240122235208.work.748-kees@kernel.org>
                   ` (2 preceding siblings ...)
  2024-01-23  0:27 ` [PATCH 29/82] rds: " Kees Cook
@ 2024-01-23  0:27 ` Kees Cook
  2024-01-26 19:31   ` Eugenio Perez Martin
  2024-01-23  0:27 ` [PATCH 64/82] netfilter: Refactor intentional wrap-around test Kees Cook
  2024-01-23  0:27 ` [PATCH 80/82] xen-netback: " Kees Cook
  5 siblings, 1 reply; 11+ 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] 11+ messages in thread

* [PATCH 64/82] netfilter: Refactor intentional wrap-around test
       [not found] <20240122235208.work.748-kees@kernel.org>
                   ` (3 preceding siblings ...)
  2024-01-23  0:27 ` [PATCH 32/82] vringh: " Kees Cook
@ 2024-01-23  0:27 ` Kees Cook
  2024-01-23 18:03   ` Florian Westphal
  2024-01-23  0:27 ` [PATCH 80/82] xen-netback: " Kees Cook
  5 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2024-01-23  0:27 UTC (permalink / raw)
  To: linux-hardening
  Cc: Kees Cook, Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	netfilter-devel, coreteam, 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 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: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netfilter-devel@vger.kernel.org
Cc: coreteam@netfilter.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/netfilter/xt_u32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
index 117d4615d668..8623fe2d97e9 100644
--- a/net/netfilter/xt_u32.c
+++ b/net/netfilter/xt_u32.c
@@ -58,11 +58,11 @@ static bool u32_match_it(const struct xt_u32 *data,
 				val >>= number;
 				break;
 			case XT_U32_AT:
-				if (at + val < at)
+				if (add_would_overflow(at, val))
 					return false;
 				at += val;
 				pos = number;
-				if (at + 4 < at || skb->len < at + 4 ||
+				if (add_would_overflow(at, 4) || skb->len < at + 4 ||
 				    pos > skb->len - at - 4)
 					return false;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 80/82] xen-netback: Refactor intentional wrap-around test
       [not found] <20240122235208.work.748-kees@kernel.org>
                   ` (4 preceding siblings ...)
  2024-01-23  0:27 ` [PATCH 64/82] netfilter: Refactor intentional wrap-around test Kees Cook
@ 2024-01-23  0:27 ` Kees Cook
  2024-01-23  7:55   ` Jan Beulich
  5 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2024-01-23  0:27 UTC (permalink / raw)
  To: linux-hardening
  Cc: Kees Cook, Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, xen-devel, 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 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: Wei Liu <wei.liu@kernel.org>
Cc: Paul Durrant <paul@xen.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: xen-devel@lists.xenproject.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/xen-netback/hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
index ff96f22648ef..69b03b4feba9 100644
--- a/drivers/net/xen-netback/hash.c
+++ b/drivers/net/xen-netback/hash.c
@@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
 		.flags = GNTCOPY_source_gref
 	}};
 
-	if ((off + len < off) || (off + len > vif->hash.size) ||
+	if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) ||
 	    len > XEN_PAGE_SIZE / sizeof(*mapping))
 		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 80/82] xen-netback: Refactor intentional wrap-around test
  2024-01-23  0:27 ` [PATCH 80/82] xen-netback: " Kees Cook
@ 2024-01-23  7:55   ` Jan Beulich
  2024-01-23 21:32     ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2024-01-23  7:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, xen-devel, netdev,
	Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel,
	linux-hardening

On 23.01.2024 01:27, Kees Cook wrote:
> --- a/drivers/net/xen-netback/hash.c
> +++ b/drivers/net/xen-netback/hash.c
> @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
>  		.flags = GNTCOPY_source_gref
>  	}};
>  
> -	if ((off + len < off) || (off + len > vif->hash.size) ||
> +	if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) ||

I'm not maintainer of this code, but if I was I would ask that the
excess parentheses be removed, to improve readability.

Jan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 64/82] netfilter: Refactor intentional wrap-around test
  2024-01-23  0:27 ` [PATCH 64/82] netfilter: Refactor intentional wrap-around test Kees Cook
@ 2024-01-23 18:03   ` Florian Westphal
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-01-23 18:03 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-hardening, Pablo Neira Ayuso, Jozsef Kadlecsik,
	Florian Westphal, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netfilter-devel, coreteam, netdev,
	Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel

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

Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 80/82] xen-netback: Refactor intentional wrap-around test
  2024-01-23  7:55   ` Jan Beulich
@ 2024-01-23 21:32     ` Kees Cook
  0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2024-01-23 21:32 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, xen-devel, netdev,
	Gustavo A. R. Silva, Bill Wendling, Justin Stitt, linux-kernel,
	linux-hardening

On Tue, Jan 23, 2024 at 08:55:44AM +0100, Jan Beulich wrote:
> On 23.01.2024 01:27, Kees Cook wrote:
> > --- a/drivers/net/xen-netback/hash.c
> > +++ b/drivers/net/xen-netback/hash.c
> > @@ -345,7 +345,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
> >  		.flags = GNTCOPY_source_gref
> >  	}};
> >  
> > -	if ((off + len < off) || (off + len > vif->hash.size) ||
> > +	if ((add_would_overflow(off, len)) || (off + len > vif->hash.size) ||
> 
> I'm not maintainer of this code, but if I was I would ask that the
> excess parentheses be removed, to improve readability.

Good call. I will adjust that. Thanks!

-Kees

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 32/82] vringh: Refactor intentional wrap-around calculation
  2024-01-23  0:27 ` [PATCH 32/82] vringh: " Kees Cook
@ 2024-01-26 19:31   ` Eugenio Perez Martin
  2024-01-26 19:42     ` Kees Cook
  0 siblings, 1 reply; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread

end of thread, other threads:[~2024-01-26 19:42 UTC | newest]

Thread overview: 11+ 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:26 ` [PATCH 12/82] ipv4: Silence intentional wrapping addition Kees Cook
2024-01-23  0:27 ` [PATCH 28/82] niu: Refactor intentional wrap-around calculation Kees Cook
2024-01-23  0:27 ` [PATCH 29/82] rds: " Kees Cook
2024-01-23  0:27 ` [PATCH 32/82] vringh: " Kees Cook
2024-01-26 19:31   ` Eugenio Perez Martin
2024-01-26 19:42     ` Kees Cook
2024-01-23  0:27 ` [PATCH 64/82] netfilter: Refactor intentional wrap-around test Kees Cook
2024-01-23 18:03   ` Florian Westphal
2024-01-23  0:27 ` [PATCH 80/82] xen-netback: " Kees Cook
2024-01-23  7:55   ` Jan Beulich
2024-01-23 21:32     ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).