* [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).