| Message ID | 20160728163714.GP36313@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Jul 28, 2016 at 12:37:14PM -0400, Jarod Wilson wrote: > 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@mellanox.com> > > >Signed-off-by: Jarod Wilson <jarod@redhat.com> > > >--- > > > 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@redhat.com> > 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@redhat.com> > --- > 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)); Nope, this is wrong, should be just data, not &data, I think. Or perhaps memset(&data->reserved, 0, sizeof(data->reserved));. Urgh, pointers...
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); }