| Message ID | 53e2bd6728136d5916e384a7840e5dc7eebff832.1729099611.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d95d9a31aceb2021084bc9b94647bc5b175e05e7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: fix integer overflow in stats | expand |
On Thu, Oct 17, 2024 at 1:27 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > Static analysis on linux-next has detected the following issue > in function virtnet_stats_ctx_init, in drivers/net/virtio_net.c : > > if (vi->device_stats_cap & VIRTIO_NET_STATS_TYPE_CVQ) { > queue_type = VIRTNET_Q_TYPE_CQ; > ctx->bitmap[queue_type] |= VIRTIO_NET_STATS_TYPE_CVQ; > ctx->desc_num[queue_type] += ARRAY_SIZE(virtnet_stats_cvq_desc); > ctx->size[queue_type] += sizeof(struct virtio_net_stats_cvq); > } > > ctx->bitmap is declared as a u32 however it is being bit-wise or'd with > VIRTIO_NET_STATS_TYPE_CVQ and this is defined as 1 << 32: > > include/uapi/linux/virtio_net.h:#define VIRTIO_NET_STATS_TYPE_CVQ (1ULL << 32) > > ..and hence the bit-wise or operation won't set any bits in ctx->bitmap > because 1ULL < 32 is too wide for a u32. > > In fact, the field is read into a u64: > > u64 offset, bitmap; > .... > bitmap = ctx->bitmap[queue_type]; > > so to fix, it is enough to make bitmap an array of u64. > > Fixes: 941168f8b40e5 ("virtio_net: support device stats") > Reported-by: "Colin King (gmail)" <colin.i.king@gmail.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- Acked-by: Jason Wang <jasowang@redhat.com> Thanks
On Wed, Oct 16, 2024 at 01:27:07PM -0400, Michael S. Tsirkin wrote: >Static analysis on linux-next has detected the following issue >in function virtnet_stats_ctx_init, in drivers/net/virtio_net.c : > > if (vi->device_stats_cap & VIRTIO_NET_STATS_TYPE_CVQ) { > queue_type = VIRTNET_Q_TYPE_CQ; > ctx->bitmap[queue_type] |= VIRTIO_NET_STATS_TYPE_CVQ; > ctx->desc_num[queue_type] += ARRAY_SIZE(virtnet_stats_cvq_desc); > ctx->size[queue_type] += sizeof(struct virtio_net_stats_cvq); > } > >ctx->bitmap is declared as a u32 however it is being bit-wise or'd with >VIRTIO_NET_STATS_TYPE_CVQ and this is defined as 1 << 32: > >include/uapi/linux/virtio_net.h:#define VIRTIO_NET_STATS_TYPE_CVQ (1ULL << 32) > >..and hence the bit-wise or operation won't set any bits in ctx->bitmap >because 1ULL < 32 is too wide for a u32. > >In fact, the field is read into a u64: > > u64 offset, bitmap; >.... > bitmap = ctx->bitmap[queue_type]; > >so to fix, it is enough to make bitmap an array of u64. > >Fixes: 941168f8b40e5 ("virtio_net: support device stats") Release with v6.10, so should we cc stable? >Reported-by: "Colin King (gmail)" <colin.i.king@gmail.com> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >--- > drivers/net/virtio_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM! Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano > >diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >index c6af18948092..b950d24b1ffa 100644 >--- a/drivers/net/virtio_net.c >+++ b/drivers/net/virtio_net.c >@@ -4112,7 +4112,7 @@ struct virtnet_stats_ctx { > u32 desc_num[3]; > > /* The actual supported stat types. */ >- u32 bitmap[3]; >+ u64 bitmap[3]; > > /* Used to calculate the reply buffer size. */ > u32 size[3]; >-- >MST > >
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Wed, 16 Oct 2024 13:27:07 -0400 you wrote: > Static analysis on linux-next has detected the following issue > in function virtnet_stats_ctx_init, in drivers/net/virtio_net.c : > > if (vi->device_stats_cap & VIRTIO_NET_STATS_TYPE_CVQ) { > queue_type = VIRTNET_Q_TYPE_CQ; > ctx->bitmap[queue_type] |= VIRTIO_NET_STATS_TYPE_CVQ; > ctx->desc_num[queue_type] += ARRAY_SIZE(virtnet_stats_cvq_desc); > ctx->size[queue_type] += sizeof(struct virtio_net_stats_cvq); > } > > [...] Here is the summary with links: - virtio_net: fix integer overflow in stats https://git.kernel.org/netdev/net/c/d95d9a31aceb You are awesome, thank you!
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index c6af18948092..b950d24b1ffa 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -4112,7 +4112,7 @@ struct virtnet_stats_ctx { u32 desc_num[3]; /* The actual supported stat types. */ - u32 bitmap[3]; + u64 bitmap[3]; /* Used to calculate the reply buffer size. */ u32 size[3];
Static analysis on linux-next has detected the following issue in function virtnet_stats_ctx_init, in drivers/net/virtio_net.c : if (vi->device_stats_cap & VIRTIO_NET_STATS_TYPE_CVQ) { queue_type = VIRTNET_Q_TYPE_CQ; ctx->bitmap[queue_type] |= VIRTIO_NET_STATS_TYPE_CVQ; ctx->desc_num[queue_type] += ARRAY_SIZE(virtnet_stats_cvq_desc); ctx->size[queue_type] += sizeof(struct virtio_net_stats_cvq); } ctx->bitmap is declared as a u32 however it is being bit-wise or'd with VIRTIO_NET_STATS_TYPE_CVQ and this is defined as 1 << 32: include/uapi/linux/virtio_net.h:#define VIRTIO_NET_STATS_TYPE_CVQ (1ULL << 32) ..and hence the bit-wise or operation won't set any bits in ctx->bitmap because 1ULL < 32 is too wide for a u32. In fact, the field is read into a u64: u64 offset, bitmap; .... bitmap = ctx->bitmap[queue_type]; so to fix, it is enough to make bitmap an array of u64. Fixes: 941168f8b40e5 ("virtio_net: support device stats") Reported-by: "Colin King (gmail)" <colin.i.king@gmail.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/net/virtio_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)