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]
...
[ 84.191982] Call Trace:
[ 84.192439] <TASK>
[ 84.192900] ? __warn+0xc9/0x1a0
[ 84.193353] ? iavf_set_rss_key+0x123/0x140 [iavf]
[ 84.193818] ? report_bug+0x12c/0x1b0
[ 84.194266] ? handle_bug+0x42/0x70
[ 84.194714] ? exc_invalid_op+0x1a/0x50
[ 84.195149] ? asm_exc_invalid_op+0x1a/0x20
[ 84.195592] ? iavf_set_rss_key+0x123/0x140 [iavf]
[ 84.196033] iavf_watchdog_task+0xb0c/0xe00 [iavf]
...
[ 84.225476] memcpy: detected field-spanning write (size 64) of single
field "vrl->lut" at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1127
(size 1)
[ 84.227190] WARNING: CPU: 27 PID: 1044 at drivers/net/ethernet/intel/
iavf/iavf_virtchnl.c:1127 iavf_set_rss_lut+0x123/0x140 [iavf]
...
[ 84.246601] Call Trace:
[ 84.247228] <TASK>
[ 84.247840] ? __warn+0xc9/0x1a0
[ 84.248263] ? iavf_set_rss_lut+0x123/0x140 [iavf]
[ 84.248698] ? report_bug+0x12c/0x1b0
[ 84.249122] ? handle_bug+0x42/0x70
[ 84.249549] ? exc_invalid_op+0x1a/0x50
[ 84.249970] ? asm_exc_invalid_op+0x1a/0x20
[ 84.250390] ? iavf_set_rss_lut+0x123/0x140 [iavf]
[ 84.250820] iavf_watchdog_task+0xb16/0xe00 [iavf]
Gustavo already tried to fix those back in 2021[0][1]. Unfortunately,
a VM can run a different kernel than the host, meaning that those
structures are sorta ABI.
However, it is possible to have proper flex arrays + struct_size()
calculations and still send the very same messages with the same sizes.
The common rule is:
elem[1] -> elem[]
size = struct_size() + <difference between the old and the new msg size>
The "old" size in the current code is calculated 3 different ways for
10 virtchnl structures total. Each commit addresses one of the ways
cumulatively instead of per-structure.
I was planning to send it to -net initially, but given that virtchnl was
renamed from i40evf and got some fat style cleanup commits in the past,
it's not very straightforward to even pick appropriate SHAs, not
speaking of automatic portability. I may send manual backports for
a couple of the latest supported kernels later on if anyone needs it
at all.
[0] https://lore.kernel.org/all/20210525230912.GA175802@embeddedor
[1] https://lore.kernel.org/all/20210525231851.GA176647@embeddedor
Alexander Lobakin (3):
virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` -
1
virtchnl: fix fake 1-elem arrays in structures allocated as `nents +
1`
virtchnl: fix fake 1-elem arrays for structures allocated as `nents`
.../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(-)
--
2.41.0
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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
From: Kees Cook <[email protected]>
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
From: Alexander Lobakin <[email protected]>
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
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 <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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 <[email protected]>
I remain a fan of _Generic uses. :)
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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 <[email protected]>
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 <[email protected]>
--
Kees Cook
From: Kees Cook <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
>
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
From: Kees Cook <[email protected]>
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 <[email protected]>
>> 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>
>>
>> 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
On 8/4/2023 9:38 AM, Alexander Lobakin wrote:
> From: Alexander Lobakin <[email protected]>
> 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
From: Alexander Lobakin <[email protected]>
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
On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
> From: Kees Cook <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> >
>
> 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
From: Tony Nguyen <[email protected]>
Date: Fri, 4 Aug 2023 11:07:14 -0700
> On 8/4/2023 9:38 AM, Alexander Lobakin wrote:
>> From: Alexander Lobakin <[email protected]>
>> 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