| Message ID | 20241104085706.13872-2-lulie@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 86a48a00efdf61197b6658e52c6140463eb313dc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: Make RSS interact properly with queue number | expand |
On Mon, Nov 04, 2024 at 04:57:03PM +0800, Philo Lu wrote: > When reading/writing virtio_net_ctrl_rss, we get the indirection table > size from vi->rss_indir_table_size, which is initialized in > virtnet_probe(). However, the actual size of indirection_table was set > as VIRTIO_NET_RSS_MAX_TABLE_LEN=128. This collision may cause issues if > the vi->rss_indir_table_size exceeds 128. > > This patch instead uses dynamic indirection table, allocated with > vi->rss after vi->rss_indir_table_size initialized. And free it in > virtnet_remove(). > > In virtnet_commit_rss_command(), sgs for rss is initialized differently > with hash_report. So indirection_table is not used if !vi->has_rss, and > then we don't need to alloc indirection_table for hash_report only uses. > > Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.") > Signed-off-by: Philo Lu <lulie@linux.alibaba.com> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 869586c17ffd..75c1ff4efd13 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -368,15 +368,16 @@ struct receive_queue { > * because table sizes may be differ according to the device configuration. > */ > #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40 > -#define VIRTIO_NET_RSS_MAX_TABLE_LEN 128 > struct virtio_net_ctrl_rss { > u32 hash_types; > u16 indirection_table_mask; > u16 unclassified_queue; > - u16 indirection_table[VIRTIO_NET_RSS_MAX_TABLE_LEN]; > + u16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */ > u16 max_tx_vq; > u8 hash_key_length; > u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE]; > + > + u16 *indirection_table; > }; > > /* Control VQ buffers: protected by the rtnl lock */ > @@ -512,6 +513,25 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb, > struct page *page, void *buf, > int len, int truesize); > > +static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size) > +{ > + if (!indir_table_size) { > + rss->indirection_table = NULL; > + return 0; > + } > + > + rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL); > + if (!rss->indirection_table) > + return -ENOMEM; > + > + return 0; > +} > + > +static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss) > +{ > + kfree(rss->indirection_table); > +} > + > static bool is_xdp_frame(void *ptr) > { > return (unsigned long)ptr & VIRTIO_XDP_FLAG; > @@ -3828,11 +3848,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi) > /* prepare sgs */ > sg_init_table(sgs, 4); > > - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table); > + sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved); > sg_set_buf(&sgs[0], &vi->rss, sg_buf_size); > > - sg_buf_size = sizeof(uint16_t) * (vi->rss.indirection_table_mask + 1); > - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size); > + if (vi->has_rss) { > + sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size; > + sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size); > + } else { > + sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t)); > + } > > sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key) > - offsetof(struct virtio_net_ctrl_rss, max_tx_vq); > @@ -6420,6 +6444,9 @@ static int virtnet_probe(struct virtio_device *vdev) > virtio_cread16(vdev, offsetof(struct virtio_net_config, > rss_max_indirection_table_length)); > } > + err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size); > + if (err) > + goto free; > > if (vi->has_rss || vi->has_rss_hash_report) { > vi->rss_key_size = > @@ -6674,6 +6701,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > remove_vq_common(vi); > > + rss_indirection_table_free(&vi->rss); > + > free_netdev(vi->dev); > } > I'm not an expert on virtio, so I don't feel comfortable giving a Reviewed-by, but this does seem to fix a potential out of bounds access in virtnet_init_default_rss if rss_indir_table_size were larger than VIRTIO_NET_RSS_MAX_TABLE_LEN (128). Acked-by: Joe Damato <jdamato@fastly.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 869586c17ffd..75c1ff4efd13 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -368,15 +368,16 @@ struct receive_queue { * because table sizes may be differ according to the device configuration. */ #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40 -#define VIRTIO_NET_RSS_MAX_TABLE_LEN 128 struct virtio_net_ctrl_rss { u32 hash_types; u16 indirection_table_mask; u16 unclassified_queue; - u16 indirection_table[VIRTIO_NET_RSS_MAX_TABLE_LEN]; + u16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */ u16 max_tx_vq; u8 hash_key_length; u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE]; + + u16 *indirection_table; }; /* Control VQ buffers: protected by the rtnl lock */ @@ -512,6 +513,25 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb, struct page *page, void *buf, int len, int truesize); +static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size) +{ + if (!indir_table_size) { + rss->indirection_table = NULL; + return 0; + } + + rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL); + if (!rss->indirection_table) + return -ENOMEM; + + return 0; +} + +static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss) +{ + kfree(rss->indirection_table); +} + static bool is_xdp_frame(void *ptr) { return (unsigned long)ptr & VIRTIO_XDP_FLAG; @@ -3828,11 +3848,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi) /* prepare sgs */ sg_init_table(sgs, 4); - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table); + sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved); sg_set_buf(&sgs[0], &vi->rss, sg_buf_size); - sg_buf_size = sizeof(uint16_t) * (vi->rss.indirection_table_mask + 1); - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size); + if (vi->has_rss) { + sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size; + sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size); + } else { + sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t)); + } sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key) - offsetof(struct virtio_net_ctrl_rss, max_tx_vq); @@ -6420,6 +6444,9 @@ static int virtnet_probe(struct virtio_device *vdev) virtio_cread16(vdev, offsetof(struct virtio_net_config, rss_max_indirection_table_length)); } + err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size); + if (err) + goto free; if (vi->has_rss || vi->has_rss_hash_report) { vi->rss_key_size = @@ -6674,6 +6701,8 @@ static void virtnet_remove(struct virtio_device *vdev) remove_vq_common(vi); + rss_indirection_table_free(&vi->rss); + free_netdev(vi->dev); }
When reading/writing virtio_net_ctrl_rss, we get the indirection table size from vi->rss_indir_table_size, which is initialized in virtnet_probe(). However, the actual size of indirection_table was set as VIRTIO_NET_RSS_MAX_TABLE_LEN=128. This collision may cause issues if the vi->rss_indir_table_size exceeds 128. This patch instead uses dynamic indirection table, allocated with vi->rss after vi->rss_indir_table_size initialized. And free it in virtnet_remove(). In virtnet_commit_rss_command(), sgs for rss is initialized differently with hash_report. So indirection_table is not used if !vi->has_rss, and then we don't need to alloc indirection_table for hash_report only uses. Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.") Signed-off-by: Philo Lu <lulie@linux.alibaba.com> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/net/virtio_net.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)