[Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Local 'netcfg' variable in 'virtio_net_get_config' routine was
not initialised. It could leak uninitialised 'netcfg.mtu' field
memory. Initialise 'netcfg' to avoid it.

Reported-by: Azureyang <azureyang@tencent.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/virtio-net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 01f1351..cb5b3dc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -72,7 +72,7 @@ static int vq2q(int queue_index)
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
-    struct virtio_net_config netcfg;
+    struct virtio_net_config netcfg = {};
 
     virtio_stw_p(vdev, &netcfg.status, n->status);
     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[Jason Wang]

On 2016年12月02日 19:28, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Local 'netcfg' variable in 'virtio_net_get_config' routine was
> not initialised. It could leak uninitialised 'netcfg.mtu' field
> memory. Initialise 'netcfg' to avoid it.
>
> Reported-by: Azureyang <azureyang@tencent.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/net/virtio-net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 01f1351..cb5b3dc 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -72,7 +72,7 @@ static int vq2q(int queue_index)
>   static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>   {
>       VirtIONet *n = VIRTIO_NET(vdev);
> -    struct virtio_net_config netcfg;
> +    struct virtio_net_config netcfg = {};
>   
>       virtio_stw_p(vdev, &netcfg.status, n->status);
>       virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);

Good catch but since mtu patch wasn't accepted so mtu were in fact not 
exposed to guest.

(FYI, you can have a look at Maxime patch, he did a stw_p here()).

Thanks

Re: [Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[P J P]

  Hello Jason,

+-- On Mon, 5 Dec 2016, Jason Wang wrote --+
| > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
| > index 01f1351..cb5b3dc 100644
| > --- a/hw/net/virtio-net.c
| > +++ b/hw/net/virtio-net.c
| > @@ -72,7 +72,7 @@ static int vq2q(int queue_index)
| >   static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
| >   {
| >       VirtIONet *n = VIRTIO_NET(vdev);
| > -    struct virtio_net_config netcfg;
| > +    struct virtio_net_config netcfg = {};
| >   
| >       virtio_stw_p(vdev, &netcfg.status, n->status);
| >       virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
| 
| Good catch but since mtu patch wasn't accepted so mtu were in fact not exposed
| to guest.

'mtu' appears to have been added by commit 'dbdfea9226c9d0bdd', could you 
pleae confirm?

| (FYI, you can have a look at Maxime patch, he did a stw_p here()).

  +    virtio_stw_p(vdev, &netcfg.mtu, n->mtu);
  +

  Yes, but this isn't accepted yet, is it?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[Jason Wang]

On 2016年12月05日 17:10, P J P wrote:
>    Hello Jason,
>
> +-- On Mon, 5 Dec 2016, Jason Wang wrote --+
> | > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> | > index 01f1351..cb5b3dc 100644
> | > --- a/hw/net/virtio-net.c
> | > +++ b/hw/net/virtio-net.c
> | > @@ -72,7 +72,7 @@ static int vq2q(int queue_index)
> | >   static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> | >   {
> | >       VirtIONet *n = VIRTIO_NET(vdev);
> | > -    struct virtio_net_config netcfg;
> | > +    struct virtio_net_config netcfg = {};
> | >
> | >       virtio_stw_p(vdev, &netcfg.status, n->status);
> | >       virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
> |
> | Good catch but since mtu patch wasn't accepted so mtu were in fact not exposed
> | to guest.
>
> 'mtu' appears to have been added by commit 'dbdfea9226c9d0bdd', could you
> pleae confirm?

Yes.

>
> | (FYI, you can have a look at Maxime patch, he did a stw_p here()).
>
>    +    virtio_stw_p(vdev, &netcfg.mtu, n->mtu);
>    +
>
>    Yes, but this isn't accepted yet, is it?
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

See feature_sizes[] in virtio-net.c, we won't expose mtu to guest until 
MTU feature were negotiated.

Thanks

Re: [Qemu-devel] [PATCH] net: virtio-net: initialise local 'netcfg' variable

[P J P]

  Hello Jason,

+-- On Mon, 5 Dec 2016, Jason Wang wrote --+
| See feature_sizes[] in virtio-net.c, we won't expose mtu to guest until MTU 
| feature were negotiated.

  Oh, it updates config size without the 'mtu' field during realise, okay. 
Still IMO having an initialiser({}) is better. But upto you.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F