On Wed, Nov 12, 2025 at 01:34:28PM -0600, Daniel Jurgens wrote:
...
> +static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
> +{
> + size_t ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data) +
> + sizeof(struct virtio_net_ff_selector) *
> + VIRTIO_NET_FF_MASK_TYPE_MAX;
> + struct virtio_admin_cmd_query_cap_id_result *cap_id_list;
> + struct virtio_net_ff_selector *sel;
> + size_t real_ff_mask_size;
> + int err;
> + int i;
> +
> + if (!vdev->config->admin_cmd_exec)
> + return -EOPNOTSUPP;
> +
> + cap_id_list = kzalloc(sizeof(*cap_id_list), GFP_KERNEL);
> + if (!cap_id_list)
> + return -ENOMEM;
> +
> + err = virtio_admin_cap_id_list_query(vdev, cap_id_list);
> + if (err)
> + goto err_cap_list;
> +
> + if (!(VIRTIO_CAP_IN_LIST(cap_id_list,
> + VIRTIO_NET_FF_RESOURCE_CAP) &&
> + VIRTIO_CAP_IN_LIST(cap_id_list,
> + VIRTIO_NET_FF_SELECTOR_CAP) &&
> + VIRTIO_CAP_IN_LIST(cap_id_list,
> + VIRTIO_NET_FF_ACTION_CAP))) {
> + err = -EOPNOTSUPP;
> + goto err_cap_list;
> + }
> +
> + ff->ff_caps = kzalloc(sizeof(*ff->ff_caps), GFP_KERNEL);
> + if (!ff->ff_caps) {
> + err = -ENOMEM;
> + goto err_cap_list;
> + }
> +
> + err = virtio_admin_cap_get(vdev,
> + VIRTIO_NET_FF_RESOURCE_CAP,
> + ff->ff_caps,
> + sizeof(*ff->ff_caps));
> +
> + if (err)
> + goto err_ff;
> +
> + /* VIRTIO_NET_FF_MASK_TYPE start at 1 */
> + for (i = 1; i <= VIRTIO_NET_FF_MASK_TYPE_MAX; i++)
> + ff_mask_size += get_mask_size(i);
> +
> + ff->ff_mask = kzalloc(ff_mask_size, GFP_KERNEL);
> + if (!ff->ff_mask) {
> + err = -ENOMEM;
> + goto err_ff;
> + }
> +
> + err = virtio_admin_cap_get(vdev,
> + VIRTIO_NET_FF_SELECTOR_CAP,
> + ff->ff_mask,
> + ff_mask_size);
> +
> + if (err)
> + goto err_ff_mask;
> +
> + ff->ff_actions = kzalloc(sizeof(*ff->ff_actions) +
> + VIRTIO_NET_FF_ACTION_MAX,
> + GFP_KERNEL);
> + if (!ff->ff_actions) {
> + err = -ENOMEM;
> + goto err_ff_mask;
> + }
> +
> + err = virtio_admin_cap_get(vdev,
> + VIRTIO_NET_FF_ACTION_CAP,
> + ff->ff_actions,
> + sizeof(*ff->ff_actions) +
> VIRTIO_NET_FF_ACTION_MAX);
> +
> + if (err)
> + goto err_ff_action;
> +
> + err = virtio_admin_cap_set(vdev,
> + VIRTIO_NET_FF_RESOURCE_CAP,
> + ff->ff_caps,
> + sizeof(*ff->ff_caps));
> + if (err)
> + goto err_ff_action;
> +
> + real_ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data);
> + sel = (void *)&ff->ff_mask->selectors[0];
> +
> + for (i = 0; i < ff->ff_mask->count; i++) {
> + if (sel->length > MAX_SEL_LEN) {
> + err = -EINVAL;
> + goto err_ff_action;
> + }
> + real_ff_mask_size += sizeof(struct virtio_net_ff_selector) +
> sel->length;
> + sel = (void *)sel + sizeof(*sel) + sel->length;
> + }
Hi Daniel,
I'm not sure that the bounds checking in the loop above is adequate.
For example, if ff->ff_mask->count is larger than expected.
Or sel->length returns MAX_SEL_LEN each time then it seems
than sel could overrun the space allocated for ff->ff_mask.
Flagged by Claude Code with https://github.com/masoncl/review-prompts/
> +
> + if (real_ff_mask_size > ff_mask_size) {
> + err = -EINVAL;
> + goto err_ff_action;
> + }
> +
> + err = virtio_admin_cap_set(vdev,
> + VIRTIO_NET_FF_SELECTOR_CAP,
> + ff->ff_mask,
> + real_ff_mask_size);
> + if (err)
> + goto err_ff_action;
> +
> + err = virtio_admin_cap_set(vdev,
> + VIRTIO_NET_FF_ACTION_CAP,
> + ff->ff_actions,
> + sizeof(*ff->ff_actions) +
> VIRTIO_NET_FF_ACTION_MAX);
> + if (err)
> + goto err_ff_action;
> +
> + ff->vdev = vdev;
> + ff->ff_supported = true;
> +
> + kfree(cap_id_list);
> +
> + return 0;
> +
> +err_ff_action:
> + kfree(ff->ff_actions);
> + ff->ff_actions = NULL;
> +err_ff_mask:
> + kfree(ff->ff_mask);
> + ff->ff_mask = NULL;
> +err_ff:
> + kfree(ff->ff_caps);
> + ff->ff_caps = NULL;
> +err_cap_list:
> + kfree(cap_id_list);
> +
> + return err;
> +}
...
