* [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-07-28 15:52 [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
@ 2023-07-28 15:52 ` Alexander Lobakin
2023-07-28 22:43 ` Kees Cook
2023-08-04 8:27 ` Kees Cook
2023-07-28 15:52 ` [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1` Alexander Lobakin
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Alexander Lobakin @ 2023-07-28 15:52 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Alexander Lobakin, Larysa Zaremba, Andy Shevchenko,
Gustavo A. R. Silva, Kees Cook, netdev, linux-hardening,
intel-wired-lan, linux-kernel
The two most problematic virtchnl structures are virtchnl_rss_key and
virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
allocating / checking, the actual size is calculated as `sizeof +
nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
of such structure with proper flex array, it's two bytes larger due to
the padding. That said, their size is always 1 byte larger unless
there are no tail elements -- then it's +2 bytes.
Add virtchnl_struct_size() macro which will handle this case (and later
other cases as well). Make its calling conv the same as we call
struct_size() to allow it to be drop-in, even though it's unlikely to
become possible to switch to generic API. The macro will calculate a
proper size of a structure with a flex array at the end, so that it
becomes transparent for the compilers, but add the difference from the
old values, so that the real size of sorta-ABI-messages doesn't change.
Use it on the allocation side in IAVF and the receiving side (defined
as static inline in virtchnl.h) for the mentioned two structures.
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 6 ++--
include/linux/avf/virtchnl.h | 31 ++++++++++++++-----
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index be3c007ce90a..10f03054a603 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1085,8 +1085,7 @@ void iavf_set_rss_key(struct iavf_adapter *adapter)
adapter->current_op);
return;
}
- len = sizeof(struct virtchnl_rss_key) +
- (adapter->rss_key_size * sizeof(u8)) - 1;
+ len = virtchnl_struct_size(vrk, key, adapter->rss_key_size);
vrk = kzalloc(len, GFP_KERNEL);
if (!vrk)
return;
@@ -1117,8 +1116,7 @@ void iavf_set_rss_lut(struct iavf_adapter *adapter)
adapter->current_op);
return;
}
- len = sizeof(struct virtchnl_rss_lut) +
- (adapter->rss_lut_size * sizeof(u8)) - 1;
+ len = virtchnl_struct_size(vrl, lut, adapter->rss_lut_size);
vrl = kzalloc(len, GFP_KERNEL);
if (!vrl)
return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index c15221dcb75e..3ab207c14809 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -866,18 +866,20 @@ VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_promisc_info);
struct virtchnl_rss_key {
u16 vsi_id;
u16 key_len;
- u8 key[1]; /* RSS hash key, packed bytes */
+ u8 key[]; /* RSS hash key, packed bytes */
};
-VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rss_key);
+#define virtchnl_rss_key_LEGACY_SIZEOF 6
struct virtchnl_rss_lut {
u16 vsi_id;
u16 lut_entries;
- u8 lut[1]; /* RSS lookup table */
+ u8 lut[]; /* RSS lookup table */
};
-VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_lut);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rss_lut);
+#define virtchnl_rss_lut_LEGACY_SIZEOF 6
/* VIRTCHNL_OP_GET_RSS_HENA_CAPS
* VIRTCHNL_OP_SET_RSS_HENA
@@ -1367,6 +1369,17 @@ struct virtchnl_fdir_del {
VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
+#define __vss_byone(p, member, count, old) \
+ (struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
+
+#define __vss(type, func, p, member, count) \
+ struct type: func(p, member, count, type##_LEGACY_SIZEOF)
+
+#define virtchnl_struct_size(p, m, c) \
+ _Generic(*p, \
+ __vss(virtchnl_rss_key, __vss_byone, p, m, c), \
+ __vss(virtchnl_rss_lut, __vss_byone, p, m, c))
+
/**
* virtchnl_vc_validate_vf_msg
* @ver: Virtchnl version info
@@ -1479,19 +1492,21 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
}
break;
case VIRTCHNL_OP_CONFIG_RSS_KEY:
- valid_len = sizeof(struct virtchnl_rss_key);
+ valid_len = virtchnl_rss_key_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_rss_key *vrk =
(struct virtchnl_rss_key *)msg;
- valid_len += vrk->key_len - 1;
+ valid_len = virtchnl_struct_size(vrk, key,
+ vrk->key_len);
}
break;
case VIRTCHNL_OP_CONFIG_RSS_LUT:
- valid_len = sizeof(struct virtchnl_rss_lut);
+ valid_len = virtchnl_rss_lut_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_rss_lut *vrl =
(struct virtchnl_rss_lut *)msg;
- valid_len += vrl->lut_entries - 1;
+ valid_len = virtchnl_struct_size(vrl, lut,
+ vrl->lut_entries);
}
break;
case VIRTCHNL_OP_GET_RSS_HENA_CAPS:
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-07-28 15:52 ` [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1 Alexander Lobakin
@ 2023-07-28 22:43 ` Kees Cook
2023-08-01 13:08 ` Alexander Lobakin
2023-08-04 8:27 ` Kees Cook
1 sibling, 1 reply; 21+ messages in thread
From: Kees Cook @ 2023-07-28 22:43 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> The two most problematic virtchnl structures are virtchnl_rss_key and
> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> allocating / checking, the actual size is calculated as `sizeof +
> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> of such structure with proper flex array, it's two bytes larger due to
> the padding. That said, their size is always 1 byte larger unless
> there are no tail elements -- then it's +2 bytes.
> Add virtchnl_struct_size() macro which will handle this case (and later
> other cases as well). Make its calling conv the same as we call
> struct_size() to allow it to be drop-in, even though it's unlikely to
> become possible to switch to generic API. The macro will calculate a
> proper size of a structure with a flex array at the end, so that it
> becomes transparent for the compilers, but add the difference from the
> old values, so that the real size of sorta-ABI-messages doesn't change.
> Use it on the allocation side in IAVF and the receiving side (defined
> as static inline in virtchnl.h) for the mentioned two structures.
This all looks workable, but it's a unique solution in the kernel. That
is fine, of course, but would it be easier to maintain/test if it went
with the union style solutions?
struct foo {
...
union {
type legacy_padding;
DECLARE_FLEX_ARRAY(type, member);
};
};
Then the size doesn't change and "member" can still be used. (i.e. no
collateral changes needed.)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-07-28 22:43 ` Kees Cook
@ 2023-08-01 13:08 ` Alexander Lobakin
0 siblings, 0 replies; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-01 13:08 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
From: Kees Cook <keescook@chromium.org>
Date: Fri, 28 Jul 2023 15:43:26 -0700
> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>> The two most problematic virtchnl structures are virtchnl_rss_key and
>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>> allocating / checking, the actual size is calculated as `sizeof +
>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>> of such structure with proper flex array, it's two bytes larger due to
>> the padding. That said, their size is always 1 byte larger unless
>> there are no tail elements -- then it's +2 bytes.
>> Add virtchnl_struct_size() macro which will handle this case (and later
>> other cases as well). Make its calling conv the same as we call
>> struct_size() to allow it to be drop-in, even though it's unlikely to
>> become possible to switch to generic API. The macro will calculate a
>> proper size of a structure with a flex array at the end, so that it
>> becomes transparent for the compilers, but add the difference from the
>> old values, so that the real size of sorta-ABI-messages doesn't change.
>> Use it on the allocation side in IAVF and the receiving side (defined
>> as static inline in virtchnl.h) for the mentioned two structures.
>
> This all looks workable, but it's a unique solution in the kernel. That
> is fine, of course, but would it be easier to maintain/test if it went
> with the union style solutions?
>
> struct foo {
> ...
> union {
> type legacy_padding;
> DECLARE_FLEX_ARRAY(type, member);
> };
> };
>
> Then the size doesn't change and "member" can still be used. (i.e. no
> collateral changes needed.)
This wouldn't work unfortunately. I mean, you'd still need weird
calculations.
Speaking of e.g. virtchnl_rss_lut, it's always
`struct_size(nents + 1) - 1`, you can't just use the pattern above and
then struct_size(). Not only legacy padding is needed, but also
calculation adjustments.
Or did you mean define the structures as above and leave the
calculations as they are? It makes me feel we can miss something that
way. With the series, all the broken structures are processed in one
place and I thought _this_ would be actually easier to maintain...
>
> -Kees
>
Thanks,
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-07-28 15:52 ` [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1 Alexander Lobakin
2023-07-28 22:43 ` Kees Cook
@ 2023-08-04 8:27 ` Kees Cook
2023-08-04 15:42 ` Alexander Lobakin
1 sibling, 1 reply; 21+ messages in thread
From: Kees Cook @ 2023-08-04 8:27 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> The two most problematic virtchnl structures are virtchnl_rss_key and
> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> allocating / checking, the actual size is calculated as `sizeof +
> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> of such structure with proper flex array, it's two bytes larger due to
> the padding. That said, their size is always 1 byte larger unless
> there are no tail elements -- then it's +2 bytes.
> Add virtchnl_struct_size() macro which will handle this case (and later
> other cases as well). Make its calling conv the same as we call
> struct_size() to allow it to be drop-in, even though it's unlikely to
> become possible to switch to generic API. The macro will calculate a
> proper size of a structure with a flex array at the end, so that it
> becomes transparent for the compilers, but add the difference from the
> old values, so that the real size of sorta-ABI-messages doesn't change.
> Use it on the allocation side in IAVF and the receiving side (defined
> as static inline in virtchnl.h) for the mentioned two structures.
>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
This is a novel approach to solving the ABI issues for a 1-elem
conversion, but I have been convinced it's a workable approach here. :)
Thanks for doing this conversion!
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-08-04 8:27 ` Kees Cook
@ 2023-08-04 15:42 ` Alexander Lobakin
2023-08-04 17:29 ` Kees Cook
0 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-04 15:42 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
From: Kees Cook <keescook@chromium.org>
Date: Fri, 4 Aug 2023 01:27:02 -0700
> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>> The two most problematic virtchnl structures are virtchnl_rss_key and
>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>> allocating / checking, the actual size is calculated as `sizeof +
>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>> of such structure with proper flex array, it's two bytes larger due to
>> the padding. That said, their size is always 1 byte larger unless
>> there are no tail elements -- then it's +2 bytes.
>> Add virtchnl_struct_size() macro which will handle this case (and later
>> other cases as well). Make its calling conv the same as we call
>> struct_size() to allow it to be drop-in, even though it's unlikely to
>> become possible to switch to generic API. The macro will calculate a
>> proper size of a structure with a flex array at the end, so that it
>> becomes transparent for the compilers, but add the difference from the
>> old values, so that the real size of sorta-ABI-messages doesn't change.
>> Use it on the allocation side in IAVF and the receiving side (defined
>> as static inline in virtchnl.h) for the mentioned two structures.
>>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>
> This is a novel approach to solving the ABI issues for a 1-elem
> conversion, but I have been convinced it's a workable approach here. :)
> Thanks for doing this conversion!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
Thanks a lot!
You gave Reviewed-by for patches #1 and #3, does it mean the whole
series or something is wrong with the patch #2? :D
Thanks,
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-08-04 15:42 ` Alexander Lobakin
@ 2023-08-04 17:29 ` Kees Cook
2023-08-04 17:33 ` Alexander Lobakin
0 siblings, 1 reply; 21+ messages in thread
From: Kees Cook @ 2023-08-04 17:29 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
> From: Kees Cook <keescook@chromium.org>
> Date: Fri, 4 Aug 2023 01:27:02 -0700
>
> > On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> >> The two most problematic virtchnl structures are virtchnl_rss_key and
> >> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> >> allocating / checking, the actual size is calculated as `sizeof +
> >> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> >> of such structure with proper flex array, it's two bytes larger due to
> >> the padding. That said, their size is always 1 byte larger unless
> >> there are no tail elements -- then it's +2 bytes.
> >> Add virtchnl_struct_size() macro which will handle this case (and later
> >> other cases as well). Make its calling conv the same as we call
> >> struct_size() to allow it to be drop-in, even though it's unlikely to
> >> become possible to switch to generic API. The macro will calculate a
> >> proper size of a structure with a flex array at the end, so that it
> >> becomes transparent for the compilers, but add the difference from the
> >> old values, so that the real size of sorta-ABI-messages doesn't change.
> >> Use it on the allocation side in IAVF and the receiving side (defined
> >> as static inline in virtchnl.h) for the mentioned two structures.
> >>
> >> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> >
> > This is a novel approach to solving the ABI issues for a 1-elem
> > conversion, but I have been convinced it's a workable approach here. :)
> > Thanks for doing this conversion!
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
>
> Thanks a lot!
> You gave Reviewed-by for patches #1 and #3, does it mean the whole
> series or something is wrong with the patch #2? :D
Hm, maybe delivery was delayed? I see it on lore:
https://lore.kernel.org/all/202308040128.667940394B@keescook/
--
Kees Cook
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-08-04 17:29 ` Kees Cook
@ 2023-08-04 17:33 ` Alexander Lobakin
2023-08-16 12:48 ` [Intel-wired-lan] " Romanowski, Rafal
0 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-04 17:33 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
From: Kees Cook <keescook@chromium.org>
Date: Fri, 4 Aug 2023 10:29:48 -0700
> On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
>> From: Kees Cook <keescook@chromium.org>
>> Date: Fri, 4 Aug 2023 01:27:02 -0700
>>
>>> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>>>> The two most problematic virtchnl structures are virtchnl_rss_key and
>>>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>>>> allocating / checking, the actual size is calculated as `sizeof +
>>>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>>>> of such structure with proper flex array, it's two bytes larger due to
>>>> the padding. That said, their size is always 1 byte larger unless
>>>> there are no tail elements -- then it's +2 bytes.
>>>> Add virtchnl_struct_size() macro which will handle this case (and later
>>>> other cases as well). Make its calling conv the same as we call
>>>> struct_size() to allow it to be drop-in, even though it's unlikely to
>>>> become possible to switch to generic API. The macro will calculate a
>>>> proper size of a structure with a flex array at the end, so that it
>>>> becomes transparent for the compilers, but add the difference from the
>>>> old values, so that the real size of sorta-ABI-messages doesn't change.
>>>> Use it on the allocation side in IAVF and the receiving side (defined
>>>> as static inline in virtchnl.h) for the mentioned two structures.
>>>>
>>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>>
>>> This is a novel approach to solving the ABI issues for a 1-elem
>>> conversion, but I have been convinced it's a workable approach here. :)
>>> Thanks for doing this conversion!
>>>
>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>>
>>
>> Thanks a lot!
>> You gave Reviewed-by for patches #1 and #3, does it mean the whole
>> series or something is wrong with the patch #2? :D
>
> Hm, maybe delivery was delayed? I see it on lore:
> https://lore.kernel.org/all/202308040128.667940394B@keescook/
>
Nevermind, my mail rules did put it in the folder other than the one
where the main thread was, sorry :s
Much thanks, I'm now a fan of _Generic() too :D
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1
2023-08-04 17:33 ` Alexander Lobakin
@ 2023-08-16 12:48 ` Romanowski, Rafal
0 siblings, 0 replies; 21+ messages in thread
From: Romanowski, Rafal @ 2023-08-16 12:48 UTC (permalink / raw)
To: Lobakin, Aleksander, Kees Cook
Cc: Andy Shevchenko, Zaremba, Larysa, netdev@vger.kernel.org,
Gustavo A. R. Silva, linux-kernel@vger.kernel.org, Eric Dumazet,
intel-wired-lan@lists.osuosl.org, linux-hardening@vger.kernel.org,
Jakub Kicinski, Paolo Abeni, David S. Miller
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Alexander Lobakin
> Sent: piątek, 4 sierpnia 2023 19:34
> To: Kees Cook <keescook@chromium.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Zaremba,
> Larysa <larysa.zaremba@intel.com>; netdev@vger.kernel.org; Gustavo A. R.
> Silva <gustavoars@kernel.org>; linux-kernel@vger.kernel.org; Eric Dumazet
> <edumazet@google.com>; intel-wired-lan@lists.osuosl.org; linux-
> hardening@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 1/3] virtchnl: fix fake 1-elem
> arrays in structs allocated as `nents + 1` - 1
>
> From: Kees Cook <keescook@chromium.org>
> Date: Fri, 4 Aug 2023 10:29:48 -0700
>
> > On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
> >> From: Kees Cook <keescook@chromium.org>
> >> Date: Fri, 4 Aug 2023 01:27:02 -0700
> >>
> >>> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> >>>> The two most problematic virtchnl structures are virtchnl_rss_key
> >>>> and virtchnl_rss_lut. Their "flex" arrays have the type of u8,
> >>>> thus, when allocating / checking, the actual size is calculated as
> >>>> `sizeof + nents - 1 byte`. But their sizeof() is not 1 byte larger
> >>>> than the size of such structure with proper flex array, it's two
> >>>> bytes larger due to the padding. That said, their size is always 1
> >>>> byte larger unless there are no tail elements -- then it's +2 bytes.
> >>>> Add virtchnl_struct_size() macro which will handle this case (and
> >>>> later other cases as well). Make its calling conv the same as we
> >>>> call
> >>>> struct_size() to allow it to be drop-in, even though it's unlikely
> >>>> to become possible to switch to generic API. The macro will
> >>>> calculate a proper size of a structure with a flex array at the
> >>>> end, so that it becomes transparent for the compilers, but add the
> >>>> difference from the old values, so that the real size of sorta-ABI-
> messages doesn't change.
> >>>> Use it on the allocation side in IAVF and the receiving side
> >>>> (defined as static inline in virtchnl.h) for the mentioned two structures.
> >>>>
> >>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> >>>
> >>> This is a novel approach to solving the ABI issues for a 1-elem
> >>> conversion, but I have been convinced it's a workable approach here.
> >>> :) Thanks for doing this conversion!
> >>>
> >>> Reviewed-by: Kees Cook <keescook@chromium.org>
> >>>
> >>
> >> Thanks a lot!
> >> You gave Reviewed-by for patches #1 and #3, does it mean the whole
> >> series or something is wrong with the patch #2? :D
> >
> > Hm, maybe delivery was delayed? I see it on lore:
> > https://lore.kernel.org/all/202308040128.667940394B@keescook/
> >
>
> Nevermind, my mail rules did put it in the folder other than the one where
> the main thread was, sorry :s Much thanks, I'm now a fan of _Generic() too
> :D
>
> Olek
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1`
2023-07-28 15:52 [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
2023-07-28 15:52 ` [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1 Alexander Lobakin
@ 2023-07-28 15:52 ` Alexander Lobakin
2023-08-04 8:29 ` Kees Cook
2023-07-28 15:52 ` [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents` Alexander Lobakin
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-07-28 15:52 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Alexander Lobakin, Larysa Zaremba, Andy Shevchenko,
Gustavo A. R. Silva, Kees Cook, netdev, linux-hardening,
intel-wired-lan, linux-kernel
There are five virtchnl structures, which are allocated and checked in
the code as `nents + 1`, meaning that they always have memory for one
excessive element regardless of their actual number. This comes from
that their sizeof() includes space for 1 element and then they get
allocated via struct_size() or its open-coded equivalents, passing
the actual number of elements.
Expand virtchnl_struct_size() to handle such structures and replace
those 1-elem arrays with proper flex ones. Also fix several places
which open-code %IAVF_VIRTCHNL_VF_RESOURCE_SIZE. Finally, let the
virtchnl_ether_addr_list size be computed automatically when there's
no enough space for the whole list, otherwise we have to open-code
reverse struct_size() logics.
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
.../ethernet/intel/i40e/i40e_virtchnl_pf.c | 2 +-
drivers/net/ethernet/intel/iavf/iavf.h | 6 +-
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 44 +++++++-------
drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
include/linux/avf/virtchnl.h | 57 ++++++++++++-------
5 files changed, 59 insertions(+), 52 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 398fb4854cbe..030738409f76 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2103,7 +2103,7 @@ static int i40e_vc_get_vf_resources_msg(struct i40e_vf *vf, u8 *msg)
goto err;
}
- len = struct_size(vfres, vsi_res, num_vsis);
+ len = virtchnl_struct_size(vfres, vsi_res, num_vsis);
vfres = kzalloc(len, GFP_KERNEL);
if (!vfres) {
aq_ret = I40E_ERR_NO_MEMORY;
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 8cbdebc5b698..85fba85fbb23 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -92,9 +92,9 @@ struct iavf_vsi {
#define IAVF_MBPS_DIVISOR 125000 /* divisor to convert to Mbps */
#define IAVF_MBPS_QUANTA 50
-#define IAVF_VIRTCHNL_VF_RESOURCE_SIZE (sizeof(struct virtchnl_vf_resource) + \
- (IAVF_MAX_VF_VSI * \
- sizeof(struct virtchnl_vsi_resource)))
+#define IAVF_VIRTCHNL_VF_RESOURCE_SIZE \
+ virtchnl_struct_size((struct virtchnl_vf_resource *)NULL, \
+ vsi_res, IAVF_MAX_VF_VSI)
/* MAX_MSIX_Q_VECTORS of these are allocated,
* but we only use one per queue-specific vector.
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 10f03054a603..4fdac698eb38 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -215,8 +215,7 @@ int iavf_get_vf_config(struct iavf_adapter *adapter)
u16 len;
int err;
- len = sizeof(struct virtchnl_vf_resource) +
- IAVF_MAX_VF_VSI * sizeof(struct virtchnl_vsi_resource);
+ len = IAVF_VIRTCHNL_VF_RESOURCE_SIZE;
event.buf_len = len;
event.msg_buf = kzalloc(len, GFP_KERNEL);
if (!event.msg_buf)
@@ -284,7 +283,7 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
return;
}
adapter->current_op = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
- len = struct_size(vqci, qpair, pairs);
+ len = virtchnl_struct_size(vqci, qpair, pairs);
vqci = kzalloc(len, GFP_KERNEL);
if (!vqci)
return;
@@ -397,7 +396,7 @@ void iavf_map_queues(struct iavf_adapter *adapter)
q_vectors = adapter->num_msix_vectors - NONQ_VECS;
- len = struct_size(vimi, vecmap, adapter->num_msix_vectors);
+ len = virtchnl_struct_size(vimi, vecmap, adapter->num_msix_vectors);
vimi = kzalloc(len, GFP_KERNEL);
if (!vimi)
return;
@@ -476,13 +475,11 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
}
adapter->current_op = VIRTCHNL_OP_ADD_ETH_ADDR;
- len = struct_size(veal, list, count);
+ len = virtchnl_struct_size(veal, list, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add MAC changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE -
- sizeof(struct virtchnl_ether_addr_list)) /
- sizeof(struct virtchnl_ether_addr);
- len = struct_size(veal, list, count);
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(veal, list, --count);
more = true;
}
@@ -547,13 +544,11 @@ void iavf_del_ether_addrs(struct iavf_adapter *adapter)
}
adapter->current_op = VIRTCHNL_OP_DEL_ETH_ADDR;
- len = struct_size(veal, list, count);
+ len = virtchnl_struct_size(veal, list, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many delete MAC changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE -
- sizeof(struct virtchnl_ether_addr_list)) /
- sizeof(struct virtchnl_ether_addr);
- len = struct_size(veal, list, count);
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(veal, list, --count);
more = true;
}
veal = kzalloc(len, GFP_ATOMIC);
@@ -687,12 +682,12 @@ void iavf_add_vlans(struct iavf_adapter *adapter)
adapter->current_op = VIRTCHNL_OP_ADD_VLAN;
- len = sizeof(*vvfl) + (count * sizeof(u16));
+ len = virtchnl_struct_size(vvfl, vlan_id, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE - sizeof(*vvfl)) /
- sizeof(u16);
- len = sizeof(*vvfl) + (count * sizeof(u16));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl, vlan_id,
+ --count);
more = true;
}
vvfl = kzalloc(len, GFP_ATOMIC);
@@ -838,12 +833,12 @@ void iavf_del_vlans(struct iavf_adapter *adapter)
adapter->current_op = VIRTCHNL_OP_DEL_VLAN;
- len = sizeof(*vvfl) + (count * sizeof(u16));
+ len = virtchnl_struct_size(vvfl, vlan_id, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many delete VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE - sizeof(*vvfl)) /
- sizeof(u16);
- len = sizeof(*vvfl) + (count * sizeof(u16));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl, vlan_id,
+ --count);
more = true;
}
vvfl = kzalloc(len, GFP_ATOMIC);
@@ -2173,9 +2168,8 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
}
break;
case VIRTCHNL_OP_GET_VF_RESOURCES: {
- u16 len = sizeof(struct virtchnl_vf_resource) +
- IAVF_MAX_VF_VSI *
- sizeof(struct virtchnl_vsi_resource);
+ u16 len = IAVF_VIRTCHNL_VF_RESOURCE_SIZE;
+
memcpy(adapter->vf_res, msg, min(msglen, len));
iavf_validate_num_queues(adapter);
iavf_vf_parse_hw_config(&adapter->hw, adapter->vf_res);
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 625da88e7965..a5a82833cdc6 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -428,7 +428,7 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg)
goto err;
}
- len = sizeof(struct virtchnl_vf_resource);
+ len = virtchnl_struct_size(vfres, vsi_res, 0);
vfres = kzalloc(len, GFP_KERNEL);
if (!vfres) {
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 3ab207c14809..c1a20b533fc0 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -268,10 +268,11 @@ struct virtchnl_vf_resource {
u32 rss_key_size;
u32 rss_lut_size;
- struct virtchnl_vsi_resource vsi_res[1];
+ struct virtchnl_vsi_resource vsi_res[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(36, virtchnl_vf_resource);
+VIRTCHNL_CHECK_STRUCT_LEN(20, virtchnl_vf_resource);
+#define virtchnl_vf_resource_LEGACY_SIZEOF 36
/* VIRTCHNL_OP_CONFIG_TX_QUEUE
* VF sends this message to set up parameters for one TX queue.
@@ -340,10 +341,11 @@ struct virtchnl_vsi_queue_config_info {
u16 vsi_id;
u16 num_queue_pairs;
u32 pad;
- struct virtchnl_queue_pair_info qpair[1];
+ struct virtchnl_queue_pair_info qpair[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(72, virtchnl_vsi_queue_config_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vsi_queue_config_info);
+#define virtchnl_vsi_queue_config_info_LEGACY_SIZEOF 72
/* VIRTCHNL_OP_REQUEST_QUEUES
* VF sends this message to request the PF to allocate additional queues to
@@ -385,10 +387,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_vector_map);
struct virtchnl_irq_map_info {
u16 num_vectors;
- struct virtchnl_vector_map vecmap[1];
+ struct virtchnl_vector_map vecmap[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(14, virtchnl_irq_map_info);
+VIRTCHNL_CHECK_STRUCT_LEN(2, virtchnl_irq_map_info);
+#define virtchnl_irq_map_info_LEGACY_SIZEOF 14
/* VIRTCHNL_OP_ENABLE_QUEUES
* VIRTCHNL_OP_DISABLE_QUEUES
@@ -459,10 +462,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_ether_addr);
struct virtchnl_ether_addr_list {
u16 vsi_id;
u16 num_elements;
- struct virtchnl_ether_addr list[1];
+ struct virtchnl_ether_addr list[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_ether_addr_list);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_ether_addr_list);
+#define virtchnl_ether_addr_list_LEGACY_SIZEOF 12
/* VIRTCHNL_OP_ADD_VLAN
* VF sends this message to add one or more VLAN tag filters for receives.
@@ -481,10 +485,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_ether_addr_list);
struct virtchnl_vlan_filter_list {
u16 vsi_id;
u16 num_elements;
- u16 vlan_id[1];
+ u16 vlan_id[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_vlan_filter_list);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_vlan_filter_list);
+#define virtchnl_vlan_filter_list_LEGACY_SIZEOF 6
/* This enum is used for all of the VIRTCHNL_VF_OFFLOAD_VLAN_V2_CAPS related
* structures and opcodes.
@@ -1372,11 +1377,19 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
#define __vss_byone(p, member, count, old) \
(struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
+#define __vss_full(p, member, count, old) \
+ (struct_size(p, member, count) + (old - struct_size(p, member, 0)))
+
#define __vss(type, func, p, member, count) \
struct type: func(p, member, count, type##_LEGACY_SIZEOF)
#define virtchnl_struct_size(p, m, c) \
_Generic(*p, \
+ __vss(virtchnl_vf_resource, __vss_full, p, m, c), \
+ __vss(virtchnl_vsi_queue_config_info, __vss_full, p, m, c), \
+ __vss(virtchnl_irq_map_info, __vss_full, p, m, c), \
+ __vss(virtchnl_ether_addr_list, __vss_full, p, m, c), \
+ __vss(virtchnl_vlan_filter_list, __vss_full, p, m, c), \
__vss(virtchnl_rss_key, __vss_byone, p, m, c), \
__vss(virtchnl_rss_lut, __vss_byone, p, m, c))
@@ -1414,24 +1427,23 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
valid_len = sizeof(struct virtchnl_rxq_info);
break;
case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
- valid_len = sizeof(struct virtchnl_vsi_queue_config_info);
+ valid_len = virtchnl_vsi_queue_config_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_vsi_queue_config_info *vqc =
(struct virtchnl_vsi_queue_config_info *)msg;
- valid_len += (vqc->num_queue_pairs *
- sizeof(struct
- virtchnl_queue_pair_info));
+ valid_len = virtchnl_struct_size(vqc, qpair,
+ vqc->num_queue_pairs);
if (vqc->num_queue_pairs == 0)
err_msg_format = true;
}
break;
case VIRTCHNL_OP_CONFIG_IRQ_MAP:
- valid_len = sizeof(struct virtchnl_irq_map_info);
+ valid_len = virtchnl_irq_map_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_irq_map_info *vimi =
(struct virtchnl_irq_map_info *)msg;
- valid_len += (vimi->num_vectors *
- sizeof(struct virtchnl_vector_map));
+ valid_len = virtchnl_struct_size(vimi, vecmap,
+ vimi->num_vectors);
if (vimi->num_vectors == 0)
err_msg_format = true;
}
@@ -1442,23 +1454,24 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
break;
case VIRTCHNL_OP_ADD_ETH_ADDR:
case VIRTCHNL_OP_DEL_ETH_ADDR:
- valid_len = sizeof(struct virtchnl_ether_addr_list);
+ valid_len = virtchnl_ether_addr_list_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_ether_addr_list *veal =
(struct virtchnl_ether_addr_list *)msg;
- valid_len += veal->num_elements *
- sizeof(struct virtchnl_ether_addr);
+ valid_len = virtchnl_struct_size(veal, list,
+ veal->num_elements);
if (veal->num_elements == 0)
err_msg_format = true;
}
break;
case VIRTCHNL_OP_ADD_VLAN:
case VIRTCHNL_OP_DEL_VLAN:
- valid_len = sizeof(struct virtchnl_vlan_filter_list);
+ valid_len = virtchnl_vlan_filter_list_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_vlan_filter_list *vfl =
(struct virtchnl_vlan_filter_list *)msg;
- valid_len += vfl->num_elements * sizeof(u16);
+ valid_len = virtchnl_struct_size(vfl, vlan_id,
+ vfl->num_elements);
if (vfl->num_elements == 0)
err_msg_format = true;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1`
2023-07-28 15:52 ` [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1` Alexander Lobakin
@ 2023-08-04 8:29 ` Kees Cook
2023-08-16 12:49 ` [Intel-wired-lan] " Romanowski, Rafal
0 siblings, 1 reply; 21+ messages in thread
From: Kees Cook @ 2023-08-04 8:29 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
On Fri, Jul 28, 2023 at 05:52:06PM +0200, Alexander Lobakin wrote:
> There are five virtchnl structures, which are allocated and checked in
> the code as `nents + 1`, meaning that they always have memory for one
> excessive element regardless of their actual number. This comes from
> that their sizeof() includes space for 1 element and then they get
> allocated via struct_size() or its open-coded equivalents, passing
> the actual number of elements.
> Expand virtchnl_struct_size() to handle such structures and replace
> those 1-elem arrays with proper flex ones. Also fix several places
> which open-code %IAVF_VIRTCHNL_VF_RESOURCE_SIZE. Finally, let the
> virtchnl_ether_addr_list size be computed automatically when there's
> no enough space for the whole list, otherwise we have to open-code
> reverse struct_size() logics.
>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
I remain a fan of _Generic uses. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1`
2023-08-04 8:29 ` Kees Cook
@ 2023-08-16 12:49 ` Romanowski, Rafal
0 siblings, 0 replies; 21+ messages in thread
From: Romanowski, Rafal @ 2023-08-16 12:49 UTC (permalink / raw)
To: Kees Cook, Lobakin, Aleksander
Cc: Andy Shevchenko, Zaremba, Larysa, netdev@vger.kernel.org,
Gustavo A. R. Silva, linux-kernel@vger.kernel.org, Eric Dumazet,
intel-wired-lan@lists.osuosl.org, linux-hardening@vger.kernel.org,
Jakub Kicinski, Paolo Abeni, David S. Miller
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kees Cook
> Sent: piątek, 4 sierpnia 2023 10:29
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Zaremba,
> Larysa <larysa.zaremba@intel.com>; netdev@vger.kernel.org; Gustavo A. R.
> Silva <gustavoars@kernel.org>; linux-kernel@vger.kernel.org; Eric Dumazet
> <edumazet@google.com>; intel-wired-lan@lists.osuosl.org; linux-
> hardening@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 2/3] virtchnl: fix fake 1-elem
> arrays in structures allocated as `nents + 1`
>
> On Fri, Jul 28, 2023 at 05:52:06PM +0200, Alexander Lobakin wrote:
> > There are five virtchnl structures, which are allocated and checked in
> > the code as `nents + 1`, meaning that they always have memory for one
> > excessive element regardless of their actual number. This comes from
> > that their sizeof() includes space for 1 element and then they get
> > allocated via struct_size() or its open-coded equivalents, passing the
> > actual number of elements.
> > Expand virtchnl_struct_size() to handle such structures and replace
> > those 1-elem arrays with proper flex ones. Also fix several places
> > which open-code %IAVF_VIRTCHNL_VF_RESOURCE_SIZE. Finally, let the
> > virtchnl_ether_addr_list size be computed automatically when there's
> > no enough space for the whole list, otherwise we have to open-code
> > reverse struct_size() logics.
> >
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>
> I remain a fan of _Generic uses. :)
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> --
> Kees Cook
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents`
2023-07-28 15:52 [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
2023-07-28 15:52 ` [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1 Alexander Lobakin
2023-07-28 15:52 ` [PATCH net-next 2/3] virtchnl: fix fake 1-elem arrays in structures allocated as `nents + 1` Alexander Lobakin
@ 2023-07-28 15:52 ` Alexander Lobakin
2023-08-04 8:30 ` Kees Cook
2023-08-03 15:55 ` [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
2023-08-04 16:38 ` Alexander Lobakin
4 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-07-28 15:52 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Alexander Lobakin, Larysa Zaremba, Andy Shevchenko,
Gustavo A. R. Silva, Kees Cook, netdev, linux-hardening,
intel-wired-lan, linux-kernel
Finally, fix 3 structures which are allocated technically correctly,
i.e. the calculated size equals to the one that struct_size() would
return, except for sizeof(). For &virtchnl_vlan_filter_list_v2, use
the same approach when there are no enough space as taken previously
for &virtchnl_vlan_filter_list, i.e. let the maximum size be calculated
automatically instead of trying to guestimate it using maths.
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
.../ethernet/intel/i40e/i40e_virtchnl_pf.c | 7 ++--
drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 25 +++++-------
include/linux/avf/virtchnl.h | 39 ++++++++++++-------
5 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 030738409f76..90a76e927fab 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -506,6 +506,7 @@ i40e_config_rdma_qvlist(struct i40e_vf *vf,
struct virtchnl_rdma_qv_info *qv_info;
u32 v_idx, i, reg_idx, reg;
u32 next_q_idx, next_q_type;
+ size_t size;
u32 msix_vf;
int ret = 0;
@@ -521,9 +522,9 @@ i40e_config_rdma_qvlist(struct i40e_vf *vf,
}
kfree(vf->qvlist_info);
- vf->qvlist_info = kzalloc(struct_size(vf->qvlist_info, qv_info,
- qvlist_info->num_vectors - 1),
- GFP_KERNEL);
+ size = virtchnl_struct_size(vf->qvlist_info, qv_info,
+ qvlist_info->num_vectors);
+ vf->qvlist_info = kzalloc(size, GFP_KERNEL);
if (!vf->qvlist_info) {
ret = -ENOMEM;
goto err_out;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_client.c b/drivers/net/ethernet/intel/iavf/iavf_client.c
index 93c903c02c64..e6051b6355aa 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_client.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_client.c
@@ -469,8 +469,8 @@ static int iavf_client_setup_qvlist(struct iavf_info *ldev,
}
v_qvlist_info = (struct virtchnl_rdma_qvlist_info *)qvlist_info;
- msg_size = struct_size(v_qvlist_info, qv_info,
- v_qvlist_info->num_vectors - 1);
+ msg_size = virtchnl_struct_size(v_qvlist_info, qv_info,
+ v_qvlist_info->num_vectors);
adapter->client_pending |= BIT(VIRTCHNL_OP_CONFIG_RDMA_IRQ_MAP);
err = iavf_aq_send_msg_to_pf(&adapter->hw,
diff --git a/drivers/net/ethernet/intel/iavf/iavf_client.h b/drivers/net/ethernet/intel/iavf/iavf_client.h
index c5d51d7dc7cc..500269bc0f5b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_client.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_client.h
@@ -53,7 +53,7 @@ struct iavf_qv_info {
struct iavf_qvlist_info {
u32 num_vectors;
- struct iavf_qv_info qv_info[1];
+ struct iavf_qv_info qv_info[];
};
#define IAVF_CLIENT_MSIX_ALL 0xFFFFFFFF
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 4fdac698eb38..f9727e9c3d63 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -727,15 +727,12 @@ void iavf_add_vlans(struct iavf_adapter *adapter)
more = true;
}
- len = sizeof(*vvfl_v2) + ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ len = virtchnl_struct_size(vvfl_v2, filters, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE - sizeof(*vvfl_v2)) /
- sizeof(struct virtchnl_vlan_filter);
- len = sizeof(*vvfl_v2) +
- ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl_v2, filters,
+ --count);
more = true;
}
@@ -879,16 +876,12 @@ void iavf_del_vlans(struct iavf_adapter *adapter)
adapter->current_op = VIRTCHNL_OP_DEL_VLAN_V2;
- len = sizeof(*vvfl_v2) +
- ((count - 1) * sizeof(struct virtchnl_vlan_filter));
+ len = virtchnl_struct_size(vvfl_v2, filters, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE -
- sizeof(*vvfl_v2)) /
- sizeof(struct virtchnl_vlan_filter);
- len = sizeof(*vvfl_v2) +
- ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl_v2, filters,
+ --count);
more = true;
}
@@ -1492,7 +1485,7 @@ void iavf_enable_channels(struct iavf_adapter *adapter)
return;
}
- len = struct_size(vti, list, adapter->num_tc - 1);
+ len = virtchnl_struct_size(vti, list, adapter->num_tc);
vti = kzalloc(len, GFP_KERNEL);
if (!vti)
return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index c1a20b533fc0..d0807ad43f93 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -716,10 +716,11 @@ struct virtchnl_vlan_filter_list_v2 {
u16 vport_id;
u16 num_elements;
u8 pad[4];
- struct virtchnl_vlan_filter filters[1];
+ struct virtchnl_vlan_filter filters[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(40, virtchnl_vlan_filter_list_v2);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vlan_filter_list_v2);
+#define virtchnl_vlan_filter_list_v2_LEGACY_SIZEOF 40
/* VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2
* VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2
@@ -918,10 +919,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(16, virtchnl_channel_info);
struct virtchnl_tc_info {
u32 num_tc;
u32 pad;
- struct virtchnl_channel_info list[1];
+ struct virtchnl_channel_info list[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(24, virtchnl_tc_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_tc_info);
+#define virtchnl_tc_info_LEGACY_SIZEOF 24
/* VIRTCHNL_ADD_CLOUD_FILTER
* VIRTCHNL_DEL_CLOUD_FILTER
@@ -1059,10 +1061,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_rdma_qv_info);
struct virtchnl_rdma_qvlist_info {
u32 num_vectors;
- struct virtchnl_rdma_qv_info qv_info[1];
+ struct virtchnl_rdma_qv_info qv_info[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(16, virtchnl_rdma_qvlist_info);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rdma_qvlist_info);
+#define virtchnl_rdma_qvlist_info_LEGACY_SIZEOF 16
/* VF reset states - these are written into the RSTAT register:
* VFGEN_RSTAT on the VF
@@ -1377,6 +1380,9 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
#define __vss_byone(p, member, count, old) \
(struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
+#define __vss_byelem(p, member, count, old) \
+ (struct_size(p, member, count - 1) + (old - struct_size(p, member, 0)))
+
#define __vss_full(p, member, count, old) \
(struct_size(p, member, count) + (old - struct_size(p, member, 0)))
@@ -1390,6 +1396,9 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
__vss(virtchnl_irq_map_info, __vss_full, p, m, c), \
__vss(virtchnl_ether_addr_list, __vss_full, p, m, c), \
__vss(virtchnl_vlan_filter_list, __vss_full, p, m, c), \
+ __vss(virtchnl_vlan_filter_list_v2, __vss_byelem, p, m, c), \
+ __vss(virtchnl_tc_info, __vss_byelem, p, m, c), \
+ __vss(virtchnl_rdma_qvlist_info, __vss_byelem, p, m, c), \
__vss(virtchnl_rss_key, __vss_byone, p, m, c), \
__vss(virtchnl_rss_lut, __vss_byone, p, m, c))
@@ -1495,13 +1504,13 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
case VIRTCHNL_OP_RELEASE_RDMA_IRQ_MAP:
break;
case VIRTCHNL_OP_CONFIG_RDMA_IRQ_MAP:
- valid_len = sizeof(struct virtchnl_rdma_qvlist_info);
+ valid_len = virtchnl_rdma_qvlist_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_rdma_qvlist_info *qv =
(struct virtchnl_rdma_qvlist_info *)msg;
- valid_len += ((qv->num_vectors - 1) *
- sizeof(struct virtchnl_rdma_qv_info));
+ valid_len = virtchnl_struct_size(qv, qv_info,
+ qv->num_vectors);
}
break;
case VIRTCHNL_OP_CONFIG_RSS_KEY:
@@ -1534,12 +1543,12 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
valid_len = sizeof(struct virtchnl_vf_res_request);
break;
case VIRTCHNL_OP_ENABLE_CHANNELS:
- valid_len = sizeof(struct virtchnl_tc_info);
+ valid_len = virtchnl_tc_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_tc_info *vti =
(struct virtchnl_tc_info *)msg;
- valid_len += (vti->num_tc - 1) *
- sizeof(struct virtchnl_channel_info);
+ valid_len = virtchnl_struct_size(vti, list,
+ vti->num_tc);
if (vti->num_tc == 0)
err_msg_format = true;
}
@@ -1566,13 +1575,13 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
break;
case VIRTCHNL_OP_ADD_VLAN_V2:
case VIRTCHNL_OP_DEL_VLAN_V2:
- valid_len = sizeof(struct virtchnl_vlan_filter_list_v2);
+ valid_len = virtchnl_vlan_filter_list_v2_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_vlan_filter_list_v2 *vfl =
(struct virtchnl_vlan_filter_list_v2 *)msg;
- valid_len += (vfl->num_elements - 1) *
- sizeof(struct virtchnl_vlan_filter);
+ valid_len = virtchnl_struct_size(vfl, filters,
+ vfl->num_elements);
if (vfl->num_elements == 0) {
err_msg_format = true;
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents`
2023-07-28 15:52 ` [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents` Alexander Lobakin
@ 2023-08-04 8:30 ` Kees Cook
2023-08-16 12:51 ` [Intel-wired-lan] " Romanowski, Rafal
0 siblings, 1 reply; 21+ messages in thread
From: Kees Cook @ 2023-08-04 8:30 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, netdev,
linux-hardening, intel-wired-lan, linux-kernel
On Fri, Jul 28, 2023 at 05:52:07PM +0200, Alexander Lobakin wrote:
> Finally, fix 3 structures which are allocated technically correctly,
> i.e. the calculated size equals to the one that struct_size() would
> return, except for sizeof(). For &virtchnl_vlan_filter_list_v2, use
> the same approach when there are no enough space as taken previously
> for &virtchnl_vlan_filter_list, i.e. let the maximum size be calculated
> automatically instead of trying to guestimate it using maths.
>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents`
2023-08-04 8:30 ` Kees Cook
@ 2023-08-16 12:51 ` Romanowski, Rafal
0 siblings, 0 replies; 21+ messages in thread
From: Romanowski, Rafal @ 2023-08-16 12:51 UTC (permalink / raw)
To: Kees Cook, Lobakin, Aleksander
Cc: Andy Shevchenko, Zaremba, Larysa, netdev@vger.kernel.org,
Gustavo A. R. Silva, linux-kernel@vger.kernel.org, Eric Dumazet,
intel-wired-lan@lists.osuosl.org, linux-hardening@vger.kernel.org,
Jakub Kicinski, Paolo Abeni, David S. Miller
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kees Cook
> Sent: piątek, 4 sierpnia 2023 10:30
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Zaremba,
> Larysa <larysa.zaremba@intel.com>; netdev@vger.kernel.org; Gustavo A. R.
> Silva <gustavoars@kernel.org>; linux-kernel@vger.kernel.org; Eric Dumazet
> <edumazet@google.com>; intel-wired-lan@lists.osuosl.org; linux-
> hardening@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 3/3] virtchnl: fix fake 1-elem
> arrays for structures allocated as `nents`
>
> On Fri, Jul 28, 2023 at 05:52:07PM +0200, Alexander Lobakin wrote:
> > Finally, fix 3 structures which are allocated technically correctly,
> > i.e. the calculated size equals to the one that struct_size() would
> > return, except for sizeof(). For &virtchnl_vlan_filter_list_v2, use
> > the same approach when there are no enough space as taken previously
> > for &virtchnl_vlan_filter_list, i.e. let the maximum size be
> > calculated automatically instead of trying to guestimate it using maths.
> >
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> --
> Kees Cook
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays
2023-07-28 15:52 [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
` (2 preceding siblings ...)
2023-07-28 15:52 ` [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents` Alexander Lobakin
@ 2023-08-03 15:55 ` Alexander Lobakin
2023-08-04 16:38 ` Alexander Lobakin
4 siblings, 0 replies; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-03 15:55 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Dumazet, Jakub Kicinski, Paolo Abeni, Larysa Zaremba,
Andy Shevchenko, Gustavo A. R. Silva, Kees Cook, netdev,
linux-hardening, intel-wired-lan, linux-kernel
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Fri, 28 Jul 2023 17:52:04 +0200
> 6.5-rc1 started spitting warning splats when composing virtchnl
> messages, precisely on virtchnl_rss_key and virtchnl_lut:
>
> [ 84.167709] memcpy: detected field-spanning write (size 52) of single
> field "vrk->key" at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1095
> (size 1)
> [ 84.169915] WARNING: CPU: 3 PID: 11 at drivers/net/ethernet/intel/
> iavf/iavf_virtchnl.c:1095 iavf_set_rss_key+0x123/0x140 [iavf]
(tender ping :D)
Thanks,
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays
2023-07-28 15:52 [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
` (3 preceding siblings ...)
2023-08-03 15:55 ` [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays Alexander Lobakin
@ 2023-08-04 16:38 ` Alexander Lobakin
2023-08-04 18:07 ` Tony Nguyen
4 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-04 16:38 UTC (permalink / raw)
To: Tony Nguyen
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, Kees Cook,
netdev, linux-hardening, intel-wired-lan, linux-kernel
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Fri, 28 Jul 2023 17:52:04 +0200
> 6.5-rc1 started spitting warning splats when composing virtchnl
> messages, precisely on virtchnl_rss_key and virtchnl_lut:
>
> [ 84.167709] memcpy: detected field-spanning write (size 52) of single
> field "vrk->key" at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1095
> (size 1)
> [ 84.169915] WARNING: CPU: 3 PID: 11 at drivers/net/ethernet/intel/
> iavf/iavf_virtchnl.c:1095 iavf_set_rss_key+0x123/0x140 [iavf]
[...]
> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 9 +-
> drivers/net/ethernet/intel/iavf/iavf.h | 6 +-
> drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
> drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 75 +++++------
> drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
> include/linux/avf/virtchnl.h | 127 +++++++++++-------
> 7 files changed, 124 insertions(+), 101 deletions(-)
>
Tony, could you please take it via your next tree? I'd like the
validation to make sure more different host <-> guest pairs work.
(with Kees' tags, assuming he reviewed and approved the whole series, I
asked about #2 already)
Thanks,
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays
2023-08-04 16:38 ` Alexander Lobakin
@ 2023-08-04 18:07 ` Tony Nguyen
2023-08-04 18:09 ` Alexander Lobakin
0 siblings, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2023-08-04 18:07 UTC (permalink / raw)
To: Alexander Lobakin
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, Kees Cook,
netdev, linux-hardening, intel-wired-lan, linux-kernel
On 8/4/2023 9:38 AM, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Fri, 28 Jul 2023 17:52:04 +0200
>
>> 6.5-rc1 started spitting warning splats when composing virtchnl
>> messages, precisely on virtchnl_rss_key and virtchnl_lut:
>>
>> [ 84.167709] memcpy: detected field-spanning write (size 52) of single
>> field "vrk->key" at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1095
>> (size 1)
>> [ 84.169915] WARNING: CPU: 3 PID: 11 at drivers/net/ethernet/intel/
>> iavf/iavf_virtchnl.c:1095 iavf_set_rss_key+0x123/0x140 [iavf]
>
> [...]
>
>> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 9 +-
>> drivers/net/ethernet/intel/iavf/iavf.h | 6 +-
>> drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
>> drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
>> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 75 +++++------
>> drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
>> include/linux/avf/virtchnl.h | 127 +++++++++++-------
>> 7 files changed, 124 insertions(+), 101 deletions(-)
>>
>
> Tony, could you please take it via your next tree? I'd like the
> validation to make sure more different host <-> guest pairs work.
>
> (with Kees' tags, assuming he reviewed and approved the whole series, I
> asked about #2 already)
>
> Thanks,
> Olek
Ok, will apply it today. For the future if you want it through IWL, can
you tag it with the iwl-* target (and have IWL in the To)? Since this
had 'net-next' and was 'To' netdev maintainers, I took it that you
wanted it taken through netdev.
Thanks,
Tony
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays
2023-08-04 18:07 ` Tony Nguyen
@ 2023-08-04 18:09 ` Alexander Lobakin
2023-08-16 12:48 ` [Intel-wired-lan] " Romanowski, Rafal
0 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2023-08-04 18:09 UTC (permalink / raw)
To: Tony Nguyen
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Larysa Zaremba, Andy Shevchenko, Gustavo A. R. Silva, Kees Cook,
netdev, linux-hardening, intel-wired-lan, linux-kernel
From: Tony Nguyen <anthony.l.nguyen@intel.com>
Date: Fri, 4 Aug 2023 11:07:14 -0700
> On 8/4/2023 9:38 AM, Alexander Lobakin wrote:
>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>> Date: Fri, 28 Jul 2023 17:52:04 +0200
>>
>>> 6.5-rc1 started spitting warning splats when composing virtchnl
>>> messages, precisely on virtchnl_rss_key and virtchnl_lut:
>>>
>>> [ 84.167709] memcpy: detected field-spanning write (size 52) of single
>>> field "vrk->key" at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1095
>>> (size 1)
>>> [ 84.169915] WARNING: CPU: 3 PID: 11 at drivers/net/ethernet/intel/
>>> iavf/iavf_virtchnl.c:1095 iavf_set_rss_key+0x123/0x140 [iavf]
>>
>> [...]
>>
>>> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 9 +-
>>> drivers/net/ethernet/intel/iavf/iavf.h | 6 +-
>>> drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
>>> drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
>>> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 75 +++++------
>>> drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
>>> include/linux/avf/virtchnl.h | 127 +++++++++++-------
>>> 7 files changed, 124 insertions(+), 101 deletions(-)
>>>
>>
>> Tony, could you please take it via your next tree? I'd like the
>> validation to make sure more different host <-> guest pairs work.
>>
>> (with Kees' tags, assuming he reviewed and approved the whole series, I
>> asked about #2 already)
>>
>> Thanks,
>> Olek
>
> Ok, will apply it today. For the future if you want it through IWL, can
Great, thanks!
> you tag it with the iwl-* target (and have IWL in the To)? Since this
> had 'net-next' and was 'To' netdev maintainers, I took it that you
> wanted it taken through netdev.
Sure, I know, just for some reason targeted this directly to net at
first, but then realized it would be better for this to go via IWL
:clownface:
>
> Thanks,
> Tony
Thanks,
Olek
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next 0/3] virtchnl: fix fake 1-elem arrays
2023-08-04 18:09 ` Alexander Lobakin
@ 2023-08-16 12:48 ` Romanowski, Rafal
0 siblings, 0 replies; 21+ messages in thread
From: Romanowski, Rafal @ 2023-08-16 12:48 UTC (permalink / raw)
To: Lobakin, Aleksander, Nguyen, Anthony L
Cc: Andy Shevchenko, Kees Cook, Zaremba, Larysa,
netdev@vger.kernel.org, Gustavo A. R. Silva,
linux-kernel@vger.kernel.org, Eric Dumazet,
intel-wired-lan@lists.osuosl.org, linux-hardening@vger.kernel.org,
Jakub Kicinski, Paolo Abeni, David S. Miller
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Alexander Lobakin
> Sent: piątek, 4 sierpnia 2023 20:09
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Kees Cook
> <keescook@chromium.org>; Zaremba, Larysa <larysa.zaremba@intel.com>;
> netdev@vger.kernel.org; Gustavo A. R. Silva <gustavoars@kernel.org>;
> linux-kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; intel-
> wired-lan@lists.osuosl.org; linux-hardening@vger.kernel.org; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller
> <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 0/3] virtchnl: fix fake 1-elem
> arrays
>
> From: Tony Nguyen <anthony.l.nguyen@intel.com>
> Date: Fri, 4 Aug 2023 11:07:14 -0700
>
> > On 8/4/2023 9:38 AM, Alexander Lobakin wrote:
> >> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> >> Date: Fri, 28 Jul 2023 17:52:04 +0200
> >>
> >>> 6.5-rc1 started spitting warning splats when composing virtchnl
> >>> messages, precisely on virtchnl_rss_key and virtchnl_lut:
> >>>
> >>> [ 84.167709] memcpy: detected field-spanning write (size 52) of
> >>> single field "vrk->key" at
> >>> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1095
> >>> (size 1)
> >>> [ 84.169915] WARNING: CPU: 3 PID: 11 at
> >>> drivers/net/ethernet/intel/
> >>> iavf/iavf_virtchnl.c:1095 iavf_set_rss_key+0x123/0x140 [iavf]
> >>
> >> [...]
> >>
> >>> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 9 +-
> >>> drivers/net/ethernet/intel/iavf/iavf.h | 6 +-
> >>> drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
> >>> drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
> >>> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 75 +++++------
> >>> drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
> >>> include/linux/avf/virtchnl.h | 127
> >>> +++++++++++-------
> >>> 7 files changed, 124 insertions(+), 101 deletions(-)
> >>>
> >>
> >> Tony, could you please take it via your next tree? I'd like the
> >> validation to make sure more different host <-> guest pairs work.
> >>
> >> (with Kees' tags, assuming he reviewed and approved the whole series,
> >> I
> >> asked about #2 already)
> >>
> >> Thanks,
> >> Olek
> >
> > Ok, will apply it today. For the future if you want it through IWL,
> > can
>
> Great, thanks!
>
> > you tag it with the iwl-* target (and have IWL in the To)? Since this
> > had 'net-next' and was 'To' netdev maintainers, I took it that you
> > wanted it taken through netdev.
>
> Sure, I know, just for some reason targeted this directly to net at first, but
> then realized it would be better for this to go via IWL
> :clownface:
>
> >
> > Thanks,
> > Tony
>
> Thanks,
> Olek
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next 3/3] virtchnl: fix fake 1-elem arrays for structures allocated as `nents`
2023-08-16 21:06 [PATCH net-next 0/3][pull request] " Tony Nguyen
@ 2023-08-16 21:06 ` Tony Nguyen
0 siblings, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2023-08-16 21:06 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Alexander Lobakin, anthony.l.nguyen, andriy.shevchenko,
larysa.zaremba, keescook, gustavoars, Rafal Romanowski
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Finally, fix 3 structures which are allocated technically correctly,
i.e. the calculated size equals to the one that struct_size() would
return, except for sizeof(). For &virtchnl_vlan_filter_list_v2, use
the same approach when there are no enough space as taken previously
for &virtchnl_vlan_filter_list, i.e. let the maximum size be calculated
automatically instead of trying to guestimate it using maths.
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../ethernet/intel/i40e/i40e_virtchnl_pf.c | 7 ++--
drivers/net/ethernet/intel/iavf/iavf_client.c | 4 +-
drivers/net/ethernet/intel/iavf/iavf_client.h | 2 +-
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 25 +++++-------
include/linux/avf/virtchnl.h | 39 ++++++++++++-------
5 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index a6983c2d7c30..8ea1a238dcef 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -506,6 +506,7 @@ i40e_config_rdma_qvlist(struct i40e_vf *vf,
struct virtchnl_rdma_qv_info *qv_info;
u32 v_idx, i, reg_idx, reg;
u32 next_q_idx, next_q_type;
+ size_t size;
u32 msix_vf;
int ret = 0;
@@ -521,9 +522,9 @@ i40e_config_rdma_qvlist(struct i40e_vf *vf,
}
kfree(vf->qvlist_info);
- vf->qvlist_info = kzalloc(struct_size(vf->qvlist_info, qv_info,
- qvlist_info->num_vectors - 1),
- GFP_KERNEL);
+ size = virtchnl_struct_size(vf->qvlist_info, qv_info,
+ qvlist_info->num_vectors);
+ vf->qvlist_info = kzalloc(size, GFP_KERNEL);
if (!vf->qvlist_info) {
ret = -ENOMEM;
goto err_out;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_client.c b/drivers/net/ethernet/intel/iavf/iavf_client.c
index 93c903c02c64..e6051b6355aa 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_client.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_client.c
@@ -469,8 +469,8 @@ static int iavf_client_setup_qvlist(struct iavf_info *ldev,
}
v_qvlist_info = (struct virtchnl_rdma_qvlist_info *)qvlist_info;
- msg_size = struct_size(v_qvlist_info, qv_info,
- v_qvlist_info->num_vectors - 1);
+ msg_size = virtchnl_struct_size(v_qvlist_info, qv_info,
+ v_qvlist_info->num_vectors);
adapter->client_pending |= BIT(VIRTCHNL_OP_CONFIG_RDMA_IRQ_MAP);
err = iavf_aq_send_msg_to_pf(&adapter->hw,
diff --git a/drivers/net/ethernet/intel/iavf/iavf_client.h b/drivers/net/ethernet/intel/iavf/iavf_client.h
index c5d51d7dc7cc..500269bc0f5b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_client.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_client.h
@@ -53,7 +53,7 @@ struct iavf_qv_info {
struct iavf_qvlist_info {
u32 num_vectors;
- struct iavf_qv_info qv_info[1];
+ struct iavf_qv_info qv_info[];
};
#define IAVF_CLIENT_MSIX_ALL 0xFFFFFFFF
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 4fdac698eb38..f9727e9c3d63 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -727,15 +727,12 @@ void iavf_add_vlans(struct iavf_adapter *adapter)
more = true;
}
- len = sizeof(*vvfl_v2) + ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ len = virtchnl_struct_size(vvfl_v2, filters, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE - sizeof(*vvfl_v2)) /
- sizeof(struct virtchnl_vlan_filter);
- len = sizeof(*vvfl_v2) +
- ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl_v2, filters,
+ --count);
more = true;
}
@@ -879,16 +876,12 @@ void iavf_del_vlans(struct iavf_adapter *adapter)
adapter->current_op = VIRTCHNL_OP_DEL_VLAN_V2;
- len = sizeof(*vvfl_v2) +
- ((count - 1) * sizeof(struct virtchnl_vlan_filter));
+ len = virtchnl_struct_size(vvfl_v2, filters, count);
if (len > IAVF_MAX_AQ_BUF_SIZE) {
dev_warn(&adapter->pdev->dev, "Too many add VLAN changes in one request\n");
- count = (IAVF_MAX_AQ_BUF_SIZE -
- sizeof(*vvfl_v2)) /
- sizeof(struct virtchnl_vlan_filter);
- len = sizeof(*vvfl_v2) +
- ((count - 1) *
- sizeof(struct virtchnl_vlan_filter));
+ while (len > IAVF_MAX_AQ_BUF_SIZE)
+ len = virtchnl_struct_size(vvfl_v2, filters,
+ --count);
more = true;
}
@@ -1492,7 +1485,7 @@ void iavf_enable_channels(struct iavf_adapter *adapter)
return;
}
- len = struct_size(vti, list, adapter->num_tc - 1);
+ len = virtchnl_struct_size(vti, list, adapter->num_tc);
vti = kzalloc(len, GFP_KERNEL);
if (!vti)
return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index c1a20b533fc0..d0807ad43f93 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -716,10 +716,11 @@ struct virtchnl_vlan_filter_list_v2 {
u16 vport_id;
u16 num_elements;
u8 pad[4];
- struct virtchnl_vlan_filter filters[1];
+ struct virtchnl_vlan_filter filters[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(40, virtchnl_vlan_filter_list_v2);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_vlan_filter_list_v2);
+#define virtchnl_vlan_filter_list_v2_LEGACY_SIZEOF 40
/* VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2
* VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2
@@ -918,10 +919,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(16, virtchnl_channel_info);
struct virtchnl_tc_info {
u32 num_tc;
u32 pad;
- struct virtchnl_channel_info list[1];
+ struct virtchnl_channel_info list[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(24, virtchnl_tc_info);
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_tc_info);
+#define virtchnl_tc_info_LEGACY_SIZEOF 24
/* VIRTCHNL_ADD_CLOUD_FILTER
* VIRTCHNL_DEL_CLOUD_FILTER
@@ -1059,10 +1061,11 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_rdma_qv_info);
struct virtchnl_rdma_qvlist_info {
u32 num_vectors;
- struct virtchnl_rdma_qv_info qv_info[1];
+ struct virtchnl_rdma_qv_info qv_info[];
};
-VIRTCHNL_CHECK_STRUCT_LEN(16, virtchnl_rdma_qvlist_info);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rdma_qvlist_info);
+#define virtchnl_rdma_qvlist_info_LEGACY_SIZEOF 16
/* VF reset states - these are written into the RSTAT register:
* VFGEN_RSTAT on the VF
@@ -1377,6 +1380,9 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
#define __vss_byone(p, member, count, old) \
(struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
+#define __vss_byelem(p, member, count, old) \
+ (struct_size(p, member, count - 1) + (old - struct_size(p, member, 0)))
+
#define __vss_full(p, member, count, old) \
(struct_size(p, member, count) + (old - struct_size(p, member, 0)))
@@ -1390,6 +1396,9 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
__vss(virtchnl_irq_map_info, __vss_full, p, m, c), \
__vss(virtchnl_ether_addr_list, __vss_full, p, m, c), \
__vss(virtchnl_vlan_filter_list, __vss_full, p, m, c), \
+ __vss(virtchnl_vlan_filter_list_v2, __vss_byelem, p, m, c), \
+ __vss(virtchnl_tc_info, __vss_byelem, p, m, c), \
+ __vss(virtchnl_rdma_qvlist_info, __vss_byelem, p, m, c), \
__vss(virtchnl_rss_key, __vss_byone, p, m, c), \
__vss(virtchnl_rss_lut, __vss_byone, p, m, c))
@@ -1495,13 +1504,13 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
case VIRTCHNL_OP_RELEASE_RDMA_IRQ_MAP:
break;
case VIRTCHNL_OP_CONFIG_RDMA_IRQ_MAP:
- valid_len = sizeof(struct virtchnl_rdma_qvlist_info);
+ valid_len = virtchnl_rdma_qvlist_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_rdma_qvlist_info *qv =
(struct virtchnl_rdma_qvlist_info *)msg;
- valid_len += ((qv->num_vectors - 1) *
- sizeof(struct virtchnl_rdma_qv_info));
+ valid_len = virtchnl_struct_size(qv, qv_info,
+ qv->num_vectors);
}
break;
case VIRTCHNL_OP_CONFIG_RSS_KEY:
@@ -1534,12 +1543,12 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
valid_len = sizeof(struct virtchnl_vf_res_request);
break;
case VIRTCHNL_OP_ENABLE_CHANNELS:
- valid_len = sizeof(struct virtchnl_tc_info);
+ valid_len = virtchnl_tc_info_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_tc_info *vti =
(struct virtchnl_tc_info *)msg;
- valid_len += (vti->num_tc - 1) *
- sizeof(struct virtchnl_channel_info);
+ valid_len = virtchnl_struct_size(vti, list,
+ vti->num_tc);
if (vti->num_tc == 0)
err_msg_format = true;
}
@@ -1566,13 +1575,13 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
break;
case VIRTCHNL_OP_ADD_VLAN_V2:
case VIRTCHNL_OP_DEL_VLAN_V2:
- valid_len = sizeof(struct virtchnl_vlan_filter_list_v2);
+ valid_len = virtchnl_vlan_filter_list_v2_LEGACY_SIZEOF;
if (msglen >= valid_len) {
struct virtchnl_vlan_filter_list_v2 *vfl =
(struct virtchnl_vlan_filter_list_v2 *)msg;
- valid_len += (vfl->num_elements - 1) *
- sizeof(struct virtchnl_vlan_filter);
+ valid_len = virtchnl_struct_size(vfl, filters,
+ vfl->num_elements);
if (vfl->num_elements == 0) {
err_msg_format = true;
--
2.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread