Re: [PATCH libmlx5 2/6] fix coverity buffer overrun warning

[Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, Jul 28, 2016 at 05:46:16PM +0300, Yishai Hadas wrote:
> On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> >In set_umr_data_seg, there's a union between a 16-byte struct and a
> >64-byte array, named data. The code then makes a memset() call on the
> >struct that is sizeof(array) - sizeof(struct) long, which results in
> >writing 48 bytes to a 16 byte container. Technically, we know this is
> >actually fine, because of the union, but to silence the warning, we can
> >just do the memset on the array instead. Same address, same result, but no
> >warning spew from coverity.
> >
> >CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >---
> > src/qp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/src/qp.c b/src/qp.c
> >index 51e1176..8bb66be 100644
> >--- a/src/qp.c
> >+++ b/src/qp.c
> >@@ -426,7 +426,7 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
> > 	data->klm.mkey = htonl(bind_info->mr->lkey);
> > 	data->klm.address = htonll(bind_info->addr);
> >
> >-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
> >+	memset(&data->reserved + 1, 0, sizeof(data->reserved) -
> > 	       sizeof(data->klm));
> 
> As you pointed out this is false alarm, code is correct.
> 
> Your suggestion seems wrong as it skipped size of 'reserved' instead
> of size of 'klm' (i.e. 16 bytes), isn't it ?

&data->klm + 1 and &data->reserved + 1 should point to the same location,
no? Must admit, I'm a little hazy on how a union pointer works.

Regardless, I think this might be much more straight-forward if we did
something like this:

>From 43e845cc1055e4f86a2e9c78e5a3eae2c1e915c4 Mon Sep 17 00:00:00 2001
From: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Wed, 27 Jul 2016 11:35:02 -0400
Subject: [PATCH v2 libmlx5 2/6] fix coverity buffer overrun warning

In set_umr_data_seg, there's a union between a 16-byte struct and a
64-byte array, named data. The code then makes a memset() call on the
struct that is sizeof(array) - sizeof(struct) long, which results in
writing 48 bytes to a 16 byte container. Technically, we know this is
actually fine, because of the union, but to silence the warning and
simplify the pointer math and union gymnastics, simply zero out the entire
storage space before assigning klm bits.

Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/qp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qp.c b/src/qp.c
index 51e1176..17f2c81 100644
--- a/src/qp.c
+++ b/src/qp.c
@@ -422,13 +422,12 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
 		uint8_t				reserved[64];
 	} *data = *seg;
 
+	memset(&data, 0, sizeof(*data));
+
 	data->klm.byte_count = htonl(bind_info->length);
 	data->klm.mkey = htonl(bind_info->mr->lkey);
 	data->klm.address = htonll(bind_info->addr);
 
-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
-	       sizeof(data->klm));
-
 	*seg += sizeof(*data);
 	*size += (sizeof(*data) / 16);
 }
-- 
1.8.3.1


-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html