On 27/03/2018 05:51, Jason Gunthorpe wrote:
On Tue, Mar 20, 2018 at 03:01:36PM +0200, Leon Romanovsky wrote:
+static int parse_flow_action_esp(struct ib_device *ib_dev,
+ struct ib_uverbs_file *file,
+ struct uverbs_attr_bundle *attrs,
+ struct ib_flow_action_esp_attr *esp_attr)
+{
+ struct ib_uverbs_flow_action_esp uverbs_esp = {};
+ int ret;
+
+ /* Optional param, if it doesn't exist, we get -ENOENT and skip it */
+ ret = uverbs_copy_from(&esp_attr->hdr.esn, attrs,
+ UVERBS_ATTR_FLOW_ACTION_ESP_ESN);
+ if (IS_UVERBS_COPY_ERR(ret))
+ return ret;
+
+ /* This can be called from FLOW_ACTION_ESP_MODIFY where
+ * UVERBS_ATTR_FLOW_ACTION_ESP_ATTRS is optional
+ */
+ if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_FLOW_ACTION_ESP_ATTRS)) {
+ ret = uverbs_copy_from_or_zero(&uverbs_esp, attrs,
+ UVERBS_ATTR_FLOW_ACTION_ESP_ATTRS);
+ if (ret)
+ return ret;
+
+ if (uverbs_esp.flags & ~((ESP_LAST_SUPPORTED_FLAG << 1) - 1))
+ return -EOPNOTSUPP;
+
+ esp_attr->hdr.spi = uverbs_esp.spi;
+ esp_attr->hdr.seq = uverbs_esp.seq;
+ esp_attr->hdr.tfc_pad = uverbs_esp.tfc_pad;
+ esp_attr->hdr.hard_limit_pkts = uverbs_esp.hard_limit_pkts;
+ }
+ esp_attr->hdr.flags = esp_flags_uverbs_to_verbs(attrs, uverbs_esp.flags);
+
+ if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_FLOW_ACTION_ESP_KEYMAT)) {
+ esp_attr->keymat.keymat.protocol =
+ uverbs_attr_get_enum_id(attrs,
+ UVERBS_ATTR_FLOW_ACTION_ESP_KEYMAT);
+ ret = _uverbs_copy_from_or_zero(&esp_attr->keymat.keymat + 1,
+ attrs,
+ UVERBS_ATTR_FLOW_ACTION_ESP_KEYMAT,
+ sizeof(esp_attr->keymat));
This corrupts memory. Big give-away seeing the use of the internal
accessor.
Why is all of this so ugly? It doesn't have to be so ugly.
The initial idea was that ULPs won't need to reserve space for all
keymats/replays. However, I guess that's not really a big issue and I'll
simplify the code.
Use this:
struct ib_flow_action_attrs_esp_keymats {
enum ib_uverbs_flow_action_esp_keymat protocol;
union {
struct ib_uverbs_flow_action_esp_keymat_aes_gcm aes_gcm;
} keymat;
};
Get rid of all the other keymat structs in ib_verbs.h.
Then use something sane and obviously correct:
ret = uverbs_copy_from_or_zero(
&esp_attr->keymat.keymat, attrs,
UVERBS_ATTR_FLOW_ACTION_ESP_KEYMAT);
Same issue for replay.
Ok
Jason
Thanks for the review.
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
