All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-23 15:17 ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2017-03-23 15:17 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: Arnd Bergmann, Minchan Kim, Andrew Morton, Konstantin Neumoin,
	Denis V. Lunev, Yisheng Xie, Ingo Molnar, virtualization,
	linux-kernel

The latest gcc-7.0.1 snapshot reports a new warning:

virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]

This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
>From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.

Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e1191508228..cd5c54e2003d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
 
 	available = si_mem_available();
 
+#ifdef CONFIG_VM_EVENT_COUNTERS
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
 				pages_to_bytes(events[PSWPIN]));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
 				pages_to_bytes(events[PSWPOUT]));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
 				pages_to_bytes(i.freeram));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-23 15:17 ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2017-03-23 15:17 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: Yisheng Xie, Arnd Bergmann, Konstantin Neumoin, linux-kernel,
	virtualization, Minchan Kim, Denis V. Lunev, Andrew Morton,
	Ingo Molnar

The latest gcc-7.0.1 snapshot reports a new warning:

virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]

This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.

Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e1191508228..cd5c54e2003d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
 
 	available = si_mem_available();
 
+#ifdef CONFIG_VM_EVENT_COUNTERS
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
 				pages_to_bytes(events[PSWPIN]));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
 				pages_to_bytes(events[PSWPOUT]));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
 				pages_to_bytes(i.freeram));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-23 15:17 ` Arnd Bergmann
@ 2017-03-23 15:31   ` Denis V. Lunev
  -1 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2017-03-23 15:31 UTC (permalink / raw)
  To: Arnd Bergmann, Michael S. Tsirkin, Jason Wang
  Cc: Minchan Kim, Andrew Morton, Konstantin Neumoin, Yisheng Xie,
	Ingo Molnar, virtualization, linux-kernel

On 03/23/2017 06:17 PM, Arnd Bergmann wrote:
> The latest gcc-7.0.1 snapshot reports a new warning:
>
> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>
> This seems absolutely right, so we should add an extra check to
> prevent copying uninitialized stack data into the statistics.
> From all I can tell, this has been broken since the statistics code
> was originally added in 2.6.34.
>
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Denis V. Lunev <den@openvz.org>



> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e1191508228..cd5c54e2003d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>  
>  	available = si_mem_available();
>  
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>  				pages_to_bytes(events[PSWPIN]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>  				pages_to_bytes(events[PSWPOUT]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +#endif
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>  				pages_to_bytes(i.freeram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-23 15:31   ` Denis V. Lunev
  0 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2017-03-23 15:31 UTC (permalink / raw)
  To: Arnd Bergmann, Michael S. Tsirkin, Jason Wang
  Cc: Yisheng Xie, Konstantin Neumoin, linux-kernel, virtualization,
	Minchan Kim, Andrew Morton, Ingo Molnar

On 03/23/2017 06:17 PM, Arnd Bergmann wrote:
> The latest gcc-7.0.1 snapshot reports a new warning:
>
> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>
> This seems absolutely right, so we should add an extra check to
> prevent copying uninitialized stack data into the statistics.
> From all I can tell, this has been broken since the statistics code
> was originally added in 2.6.34.
>
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Denis V. Lunev <den@openvz.org>



> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e1191508228..cd5c54e2003d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>  
>  	available = si_mem_available();
>  
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>  				pages_to_bytes(events[PSWPIN]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>  				pages_to_bytes(events[PSWPOUT]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +#endif
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>  				pages_to_bytes(i.freeram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-23 15:17 ` Arnd Bergmann
@ 2017-03-24 18:38   ` David Hildenbrand
  -1 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2017-03-24 18:38 UTC (permalink / raw)
  To: Arnd Bergmann, Michael S. Tsirkin, Jason Wang
  Cc: Yisheng Xie, Konstantin Neumoin, linux-kernel, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar,
	Ladi Prosek

On 23.03.2017 16:17, Arnd Bergmann wrote:
> The latest gcc-7.0.1 snapshot reports a new warning:
> 
> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> 
> This seems absolutely right, so we should add an extra check to
> prevent copying uninitialized stack data into the statistics.
> From all I can tell, this has been broken since the statistics code
> was originally added in 2.6.34.
> 
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e1191508228..cd5c54e2003d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>  
>  	available = si_mem_available();
>  
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>  				pages_to_bytes(events[PSWPIN]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>  				pages_to_bytes(events[PSWPOUT]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +#endif
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>  				pages_to_bytes(i.freeram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> 

CC'ing Ladi

-- 

Thanks,

David

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-24 18:38   ` David Hildenbrand
  0 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2017-03-24 18:38 UTC (permalink / raw)
  To: Arnd Bergmann, Michael S. Tsirkin, Jason Wang
  Cc: Yisheng Xie, Konstantin Neumoin, linux-kernel, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On 23.03.2017 16:17, Arnd Bergmann wrote:
> The latest gcc-7.0.1 snapshot reports a new warning:
> 
> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> 
> This seems absolutely right, so we should add an extra check to
> prevent copying uninitialized stack data into the statistics.
> From all I can tell, this has been broken since the statistics code
> was originally added in 2.6.34.
> 
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e1191508228..cd5c54e2003d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>  
>  	available = si_mem_available();
>  
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>  				pages_to_bytes(events[PSWPIN]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>  				pages_to_bytes(events[PSWPOUT]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +#endif
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>  				pages_to_bytes(i.freeram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> 

CC'ing Ladi

-- 

Thanks,

David

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-24 18:38   ` David Hildenbrand
@ 2017-03-24 20:11     ` Ladi Prosek
  -1 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-24 20:11 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Arnd Bergmann, Michael S. Tsirkin, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, linux-kernel, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> The latest gcc-7.0.1 snapshot reports a new warning:
>>
>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>>
>> This seems absolutely right, so we should add an extra check to
>> prevent copying uninitialized stack data into the statistics.
>> From all I can tell, this has been broken since the statistics code
>> was originally added in 2.6.34.
>>
>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/virtio/virtio_balloon.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 4e1191508228..cd5c54e2003d 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>
>>       available = si_mem_available();
>>
>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>>                               pages_to_bytes(events[PSWPIN]));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>>                               pages_to_bytes(events[PSWPOUT]));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> +#endif
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>>                               pages_to_bytes(i.freeram));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

This will leave four uninitialized slots in vb->stats if
CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
have

  BUG_ON(idx < VIRTIO_BALLOON_S_NR);

at the end.

You need to make sure that vb->stats is smaller, either by using
something else than VIRTIO_BALLOON_S_NR for its size or something else
than sizeof(vb->stats) as the last argument to sg_init_one in this
file.

> CC'ing Ladi

Thanks!

> --
>
> Thanks,
>
> David

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-24 20:11     ` Ladi Prosek
  0 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-24 20:11 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Yisheng Xie, Arnd Bergmann, Michael S. Tsirkin,
	Konstantin Neumoin, linux-kernel, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> The latest gcc-7.0.1 snapshot reports a new warning:
>>
>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>>
>> This seems absolutely right, so we should add an extra check to
>> prevent copying uninitialized stack data into the statistics.
>> From all I can tell, this has been broken since the statistics code
>> was originally added in 2.6.34.
>>
>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/virtio/virtio_balloon.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 4e1191508228..cd5c54e2003d 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>
>>       available = si_mem_available();
>>
>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>>                               pages_to_bytes(events[PSWPIN]));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>>                               pages_to_bytes(events[PSWPOUT]));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> +#endif
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>>                               pages_to_bytes(i.freeram));
>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

This will leave four uninitialized slots in vb->stats if
CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
have

  BUG_ON(idx < VIRTIO_BALLOON_S_NR);

at the end.

You need to make sure that vb->stats is smaller, either by using
something else than VIRTIO_BALLOON_S_NR for its size or something else
than sizeof(vb->stats) as the last argument to sg_init_one in this
file.

> CC'ing Ladi

Thanks!

> --
>
> Thanks,
>
> David

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-24 20:11     ` Ladi Prosek
@ 2017-03-24 20:40       ` Arnd Bergmann
  -1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2017-03-24 20:40 UTC (permalink / raw)
  To: Ladi Prosek
  Cc: David Hildenbrand, Michael S. Tsirkin, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, Linux Kernel Mailing List, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> On 23.03.2017 16:17, Arnd Bergmann wrote:
>>> The latest gcc-7.0.1 snapshot reports a new warning:
>>>
>>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>>>
>>> This seems absolutely right, so we should add an extra check to
>>> prevent copying uninitialized stack data into the statistics.
>>> From all I can tell, this has been broken since the statistics code
>>> was originally added in 2.6.34.
>>>
>>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>  drivers/virtio/virtio_balloon.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>> index 4e1191508228..cd5c54e2003d 100644
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>>
>>>       available = si_mem_available();
>>>
>>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>>>                               pages_to_bytes(events[PSWPIN]));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>>>                               pages_to_bytes(events[PSWPOUT]));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>>> +#endif
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>>>                               pages_to_bytes(i.freeram));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>
> This will leave four uninitialized slots in vb->stats if
> CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> have
>
>   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>
> at the end.
>
> You need to make sure that vb->stats is smaller, either by using
> something else than VIRTIO_BALLOON_S_NR for its size or something else
> than sizeof(vb->stats) as the last argument to sg_init_one in this
> file.

Ah, got it. Would one of you create a fixed patch for this, or should I?

An easy way to solve it would be to preinitialize the events array
and return zeroes to the host, but I don't know if that's the right
solution.

Another option would be to return 'idx' from update_balloon_stats,
and use that for the size:

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index cd5c54e2003d..bea3cfb88e53 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -242,7 +242,7 @@ static inline void update_stat(struct
virtio_balloon *vb, int idx,

 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)

-static void update_balloon_stats(struct virtio_balloon *vb)
+static unsigned int update_balloon_stats(struct virtio_balloon *vb)
 {
  unsigned long events[NR_VM_EVENT_ITEMS];
  struct sysinfo i;
@@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
  pages_to_bytes(i.totalram));
  update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
  pages_to_bytes(available));
+
+ return idx;
 }

 /*
@@ -294,13 +296,14 @@ static void stats_handle_request(struct
virtio_balloon *vb)
  struct virtqueue *vq;
  struct scatterlist sg;
  unsigned int len;
+ unsigned int num_stats;

- update_balloon_stats(vb);
+ num_stats = update_balloon_stats(vb);

  vq = vb->stats_vq;
  if (!virtqueue_get_buf(vq, &len))
  return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
  virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
  virtqueue_kick(vq);
 }

     Arnd

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-24 20:40       ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2017-03-24 20:40 UTC (permalink / raw)
  To: Ladi Prosek
  Cc: Yisheng Xie, Michael S. Tsirkin, Konstantin Neumoin,
	Linux Kernel Mailing List, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> On 23.03.2017 16:17, Arnd Bergmann wrote:
>>> The latest gcc-7.0.1 snapshot reports a new warning:
>>>
>>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>>>
>>> This seems absolutely right, so we should add an extra check to
>>> prevent copying uninitialized stack data into the statistics.
>>> From all I can tell, this has been broken since the statistics code
>>> was originally added in 2.6.34.
>>>
>>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>  drivers/virtio/virtio_balloon.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>> index 4e1191508228..cd5c54e2003d 100644
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>>
>>>       available = si_mem_available();
>>>
>>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>>>                               pages_to_bytes(events[PSWPIN]));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>>>                               pages_to_bytes(events[PSWPOUT]));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>>> +#endif
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>>>                               pages_to_bytes(i.freeram));
>>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>
> This will leave four uninitialized slots in vb->stats if
> CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> have
>
>   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>
> at the end.
>
> You need to make sure that vb->stats is smaller, either by using
> something else than VIRTIO_BALLOON_S_NR for its size or something else
> than sizeof(vb->stats) as the last argument to sg_init_one in this
> file.

Ah, got it. Would one of you create a fixed patch for this, or should I?

An easy way to solve it would be to preinitialize the events array
and return zeroes to the host, but I don't know if that's the right
solution.

Another option would be to return 'idx' from update_balloon_stats,
and use that for the size:

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index cd5c54e2003d..bea3cfb88e53 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -242,7 +242,7 @@ static inline void update_stat(struct
virtio_balloon *vb, int idx,

 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)

-static void update_balloon_stats(struct virtio_balloon *vb)
+static unsigned int update_balloon_stats(struct virtio_balloon *vb)
 {
  unsigned long events[NR_VM_EVENT_ITEMS];
  struct sysinfo i;
@@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
  pages_to_bytes(i.totalram));
  update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
  pages_to_bytes(available));
+
+ return idx;
 }

 /*
@@ -294,13 +296,14 @@ static void stats_handle_request(struct
virtio_balloon *vb)
  struct virtqueue *vq;
  struct scatterlist sg;
  unsigned int len;
+ unsigned int num_stats;

- update_balloon_stats(vb);
+ num_stats = update_balloon_stats(vb);

  vq = vb->stats_vq;
  if (!virtqueue_get_buf(vq, &len))
  return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
  virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
  virtqueue_kick(vq);
 }

     Arnd

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-24 20:40       ` Arnd Bergmann
@ 2017-03-24 20:59         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2017-03-24 20:59 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ladi Prosek, David Hildenbrand, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, Linux Kernel Mailing List, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
> >>> The latest gcc-7.0.1 snapshot reports a new warning:
> >>>
> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> >>>
> >>> This seems absolutely right, so we should add an extra check to
> >>> prevent copying uninitialized stack data into the statistics.
> >>> From all I can tell, this has been broken since the statistics code
> >>> was originally added in 2.6.34.
> >>>
> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >>> ---
> >>>  drivers/virtio/virtio_balloon.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >>> index 4e1191508228..cd5c54e2003d 100644
> >>> --- a/drivers/virtio/virtio_balloon.c
> >>> +++ b/drivers/virtio/virtio_balloon.c
> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >>>
> >>>       available = si_mem_available();
> >>>
> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> >>>                               pages_to_bytes(events[PSWPIN]));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> >>>                               pages_to_bytes(events[PSWPOUT]));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> >>> +#endif
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
> >>>                               pages_to_bytes(i.freeram));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> >
> > This will leave four uninitialized slots in vb->stats if
> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> > have
> >
> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
> >
> > at the end.
> >
> > You need to make sure that vb->stats is smaller, either by using
> > something else than VIRTIO_BALLOON_S_NR for its size or something else
> > than sizeof(vb->stats) as the last argument to sg_init_one in this
> > file.
> 
> Ah, got it. Would one of you create a fixed patch for this, or should I?
> 
> An easy way to solve it would be to preinitialize the events array
> and return zeroes to the host, but I don't know if that's the right
> solution.


I think that's out of spec.

> Another option would be to return 'idx' from update_balloon_stats,
> and use that for the size:
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index cd5c54e2003d..bea3cfb88e53 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -242,7 +242,7 @@ static inline void update_stat(struct
> virtio_balloon *vb, int idx,
> 
>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> 
> -static void update_balloon_stats(struct virtio_balloon *vb)
> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>  {
>   unsigned long events[NR_VM_EVENT_ITEMS];
>   struct sysinfo i;
> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>   pages_to_bytes(i.totalram));
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>   pages_to_bytes(available));
> +
> + return idx;
>  }
> 
>  /*
> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> virtio_balloon *vb)
>   struct virtqueue *vq;
>   struct scatterlist sg;
>   unsigned int len;
> + unsigned int num_stats;
> 
> - update_balloon_stats(vb);
> + num_stats = update_balloon_stats(vb);
> 
>   vq = vb->stats_vq;
>   if (!virtqueue_get_buf(vq, &len))
>   return;
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>   virtqueue_kick(vq);
>  }
> 
>      Arnd

Sounds reasonable.

-- 
MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-24 20:59         ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2017-03-24 20:59 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Yisheng Xie, Konstantin Neumoin, Linux Kernel Mailing List,
	virtualization, Minchan Kim, Denis V. Lunev, Andrew Morton,
	Ingo Molnar

On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
> >>> The latest gcc-7.0.1 snapshot reports a new warning:
> >>>
> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> >>>
> >>> This seems absolutely right, so we should add an extra check to
> >>> prevent copying uninitialized stack data into the statistics.
> >>> From all I can tell, this has been broken since the statistics code
> >>> was originally added in 2.6.34.
> >>>
> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >>> ---
> >>>  drivers/virtio/virtio_balloon.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >>> index 4e1191508228..cd5c54e2003d 100644
> >>> --- a/drivers/virtio/virtio_balloon.c
> >>> +++ b/drivers/virtio/virtio_balloon.c
> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >>>
> >>>       available = si_mem_available();
> >>>
> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> >>>                               pages_to_bytes(events[PSWPIN]));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> >>>                               pages_to_bytes(events[PSWPOUT]));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> >>> +#endif
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
> >>>                               pages_to_bytes(i.freeram));
> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> >
> > This will leave four uninitialized slots in vb->stats if
> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> > have
> >
> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
> >
> > at the end.
> >
> > You need to make sure that vb->stats is smaller, either by using
> > something else than VIRTIO_BALLOON_S_NR for its size or something else
> > than sizeof(vb->stats) as the last argument to sg_init_one in this
> > file.
> 
> Ah, got it. Would one of you create a fixed patch for this, or should I?
> 
> An easy way to solve it would be to preinitialize the events array
> and return zeroes to the host, but I don't know if that's the right
> solution.


I think that's out of spec.

> Another option would be to return 'idx' from update_balloon_stats,
> and use that for the size:
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index cd5c54e2003d..bea3cfb88e53 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -242,7 +242,7 @@ static inline void update_stat(struct
> virtio_balloon *vb, int idx,
> 
>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> 
> -static void update_balloon_stats(struct virtio_balloon *vb)
> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>  {
>   unsigned long events[NR_VM_EVENT_ITEMS];
>   struct sysinfo i;
> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>   pages_to_bytes(i.totalram));
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>   pages_to_bytes(available));
> +
> + return idx;
>  }
> 
>  /*
> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> virtio_balloon *vb)
>   struct virtqueue *vq;
>   struct scatterlist sg;
>   unsigned int len;
> + unsigned int num_stats;
> 
> - update_balloon_stats(vb);
> + num_stats = update_balloon_stats(vb);
> 
>   vq = vb->stats_vq;
>   if (!virtqueue_get_buf(vq, &len))
>   return;
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>   virtqueue_kick(vq);
>  }
> 
>      Arnd

Sounds reasonable.

-- 
MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-24 20:59         ` Michael S. Tsirkin
@ 2017-03-27 10:02           ` Ladi Prosek
  -1 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-27 10:02 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, David Hildenbrand, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, Linux Kernel Mailing List, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >>>
>> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>> >>>
>> >>> This seems absolutely right, so we should add an extra check to
>> >>> prevent copying uninitialized stack data into the statistics.
>> >>> From all I can tell, this has been broken since the statistics code
>> >>> was originally added in 2.6.34.
>> >>>
>> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >>> ---
>> >>>  drivers/virtio/virtio_balloon.c | 2 ++
>> >>>  1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >>> index 4e1191508228..cd5c54e2003d 100644
>> >>> --- a/drivers/virtio/virtio_balloon.c
>> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >>>
>> >>>       available = si_mem_available();
>> >>>
>> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >>>                               pages_to_bytes(events[PSWPIN]));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >>>                               pages_to_bytes(events[PSWPOUT]));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> >>> +#endif
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >>>                               pages_to_bytes(i.freeram));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >
>> > This will leave four uninitialized slots in vb->stats if
>> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> > have
>> >
>> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >
>> > at the end.
>> >
>> > You need to make sure that vb->stats is smaller, either by using
>> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> > file.
>>
>> Ah, got it. Would one of you create a fixed patch for this, or should I?

I will do it. If we go the route of returning 'idx' from
update_balloon_stats, my patch

   virtio_balloon: don't push uninitialized buffers to stats virtqueue

needs to be redone anyway. Thanks!

>> An easy way to solve it would be to preinitialize the events array
>> and return zeroes to the host, but I don't know if that's the right
>> solution.
>
>
> I think that's out of spec.
>
>> Another option would be to return 'idx' from update_balloon_stats,
>> and use that for the size:
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index cd5c54e2003d..bea3cfb88e53 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -242,7 +242,7 @@ static inline void update_stat(struct
>> virtio_balloon *vb, int idx,
>>
>>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>>
>> -static void update_balloon_stats(struct virtio_balloon *vb)
>> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>>  {
>>   unsigned long events[NR_VM_EVENT_ITEMS];
>>   struct sysinfo i;
>> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>   pages_to_bytes(i.totalram));
>>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>>   pages_to_bytes(available));
>> +
>> + return idx;
>>  }
>>
>>  /*
>> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> virtio_balloon *vb)
>>   struct virtqueue *vq;
>>   struct scatterlist sg;
>>   unsigned int len;
>> + unsigned int num_stats;
>>
>> - update_balloon_stats(vb);
>> + num_stats = update_balloon_stats(vb);
>>
>>   vq = vb->stats_vq;
>>   if (!virtqueue_get_buf(vq, &len))
>>   return;
>> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>>   virtqueue_kick(vq);
>>  }
>>
>>      Arnd
>
> Sounds reasonable.
>
> --
> MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-27 10:02           ` Ladi Prosek
  0 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-27 10:02 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Yisheng Xie, Arnd Bergmann, Konstantin Neumoin,
	Linux Kernel Mailing List, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >>>
>> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>> >>>
>> >>> This seems absolutely right, so we should add an extra check to
>> >>> prevent copying uninitialized stack data into the statistics.
>> >>> From all I can tell, this has been broken since the statistics code
>> >>> was originally added in 2.6.34.
>> >>>
>> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >>> ---
>> >>>  drivers/virtio/virtio_balloon.c | 2 ++
>> >>>  1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >>> index 4e1191508228..cd5c54e2003d 100644
>> >>> --- a/drivers/virtio/virtio_balloon.c
>> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >>>
>> >>>       available = si_mem_available();
>> >>>
>> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >>>                               pages_to_bytes(events[PSWPIN]));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >>>                               pages_to_bytes(events[PSWPOUT]));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> >>> +#endif
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >>>                               pages_to_bytes(i.freeram));
>> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >
>> > This will leave four uninitialized slots in vb->stats if
>> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> > have
>> >
>> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >
>> > at the end.
>> >
>> > You need to make sure that vb->stats is smaller, either by using
>> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> > file.
>>
>> Ah, got it. Would one of you create a fixed patch for this, or should I?

I will do it. If we go the route of returning 'idx' from
update_balloon_stats, my patch

   virtio_balloon: don't push uninitialized buffers to stats virtqueue

needs to be redone anyway. Thanks!

>> An easy way to solve it would be to preinitialize the events array
>> and return zeroes to the host, but I don't know if that's the right
>> solution.
>
>
> I think that's out of spec.
>
>> Another option would be to return 'idx' from update_balloon_stats,
>> and use that for the size:
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index cd5c54e2003d..bea3cfb88e53 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -242,7 +242,7 @@ static inline void update_stat(struct
>> virtio_balloon *vb, int idx,
>>
>>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>>
>> -static void update_balloon_stats(struct virtio_balloon *vb)
>> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>>  {
>>   unsigned long events[NR_VM_EVENT_ITEMS];
>>   struct sysinfo i;
>> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>>   pages_to_bytes(i.totalram));
>>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>>   pages_to_bytes(available));
>> +
>> + return idx;
>>  }
>>
>>  /*
>> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> virtio_balloon *vb)
>>   struct virtqueue *vq;
>>   struct scatterlist sg;
>>   unsigned int len;
>> + unsigned int num_stats;
>>
>> - update_balloon_stats(vb);
>> + num_stats = update_balloon_stats(vb);
>>
>>   vq = vb->stats_vq;
>>   if (!virtqueue_get_buf(vq, &len))
>>   return;
>> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>>   virtqueue_kick(vq);
>>  }
>>
>>      Arnd
>
> Sounds reasonable.
>
> --
> MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-27 10:02           ` Ladi Prosek
@ 2017-03-28 16:18             ` Michael S. Tsirkin
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2017-03-28 16:18 UTC (permalink / raw)
  To: Ladi Prosek
  Cc: Arnd Bergmann, David Hildenbrand, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, Linux Kernel Mailing List, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On Mon, Mar 27, 2017 at 12:02:33PM +0200, Ladi Prosek wrote:
> On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> >> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> >> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
> >> >>> The latest gcc-7.0.1 snapshot reports a new warning:
> >> >>>
> >> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> >> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>>
> >> >>> This seems absolutely right, so we should add an extra check to
> >> >>> prevent copying uninitialized stack data into the statistics.
> >> >>> From all I can tell, this has been broken since the statistics code
> >> >>> was originally added in 2.6.34.
> >> >>>
> >> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> >> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> >>> ---
> >> >>>  drivers/virtio/virtio_balloon.c | 2 ++
> >> >>>  1 file changed, 2 insertions(+)
> >> >>>
> >> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >> >>> index 4e1191508228..cd5c54e2003d 100644
> >> >>> --- a/drivers/virtio/virtio_balloon.c
> >> >>> +++ b/drivers/virtio/virtio_balloon.c
> >> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >> >>>
> >> >>>       available = si_mem_available();
> >> >>>
> >> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> >> >>>                               pages_to_bytes(events[PSWPIN]));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> >> >>>                               pages_to_bytes(events[PSWPOUT]));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> >> >>> +#endif
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
> >> >>>                               pages_to_bytes(i.freeram));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> >> >
> >> > This will leave four uninitialized slots in vb->stats if
> >> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> >> > have
> >> >
> >> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
> >> >
> >> > at the end.
> >> >
> >> > You need to make sure that vb->stats is smaller, either by using
> >> > something else than VIRTIO_BALLOON_S_NR for its size or something else
> >> > than sizeof(vb->stats) as the last argument to sg_init_one in this
> >> > file.
> >>
> >> Ah, got it. Would one of you create a fixed patch for this, or should I?
> 
> I will do it. If we go the route of returning 'idx' from
> update_balloon_stats, my patch
> 
>    virtio_balloon: don't push uninitialized buffers to stats virtqueue
> 
> needs to be redone anyway. Thanks!

Can you do it soon pls? I'd like to do a pull request.

> >> An easy way to solve it would be to preinitialize the events array
> >> and return zeroes to the host, but I don't know if that's the right
> >> solution.
> >
> >
> > I think that's out of spec.
> >
> >> Another option would be to return 'idx' from update_balloon_stats,
> >> and use that for the size:
> >>
> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >> index cd5c54e2003d..bea3cfb88e53 100644
> >> --- a/drivers/virtio/virtio_balloon.c
> >> +++ b/drivers/virtio/virtio_balloon.c
> >> @@ -242,7 +242,7 @@ static inline void update_stat(struct
> >> virtio_balloon *vb, int idx,
> >>
> >>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> >>
> >> -static void update_balloon_stats(struct virtio_balloon *vb)
> >> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> >>  {
> >>   unsigned long events[NR_VM_EVENT_ITEMS];
> >>   struct sysinfo i;
> >> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >>   pages_to_bytes(i.totalram));
> >>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> >>   pages_to_bytes(available));
> >> +
> >> + return idx;
> >>  }
> >>
> >>  /*
> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> >> virtio_balloon *vb)
> >>   struct virtqueue *vq;
> >>   struct scatterlist sg;
> >>   unsigned int len;
> >> + unsigned int num_stats;
> >>
> >> - update_balloon_stats(vb);
> >> + num_stats = update_balloon_stats(vb);
> >>
> >>   vq = vb->stats_vq;
> >>   if (!virtqueue_get_buf(vq, &len))
> >>   return;
> >> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> >> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
> >>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> >>   virtqueue_kick(vq);
> >>  }
> >>
> >>      Arnd
> >
> > Sounds reasonable.
> >
> > --
> > MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-28 16:18             ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2017-03-28 16:18 UTC (permalink / raw)
  To: Ladi Prosek
  Cc: Yisheng Xie, Arnd Bergmann, Konstantin Neumoin,
	Linux Kernel Mailing List, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Mon, Mar 27, 2017 at 12:02:33PM +0200, Ladi Prosek wrote:
> On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
> >> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
> >> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
> >> >>> The latest gcc-7.0.1 snapshot reports a new warning:
> >> >>>
> >> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> >> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
> >> >>>
> >> >>> This seems absolutely right, so we should add an extra check to
> >> >>> prevent copying uninitialized stack data into the statistics.
> >> >>> From all I can tell, this has been broken since the statistics code
> >> >>> was originally added in 2.6.34.
> >> >>>
> >> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> >> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> >>> ---
> >> >>>  drivers/virtio/virtio_balloon.c | 2 ++
> >> >>>  1 file changed, 2 insertions(+)
> >> >>>
> >> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >> >>> index 4e1191508228..cd5c54e2003d 100644
> >> >>> --- a/drivers/virtio/virtio_balloon.c
> >> >>> +++ b/drivers/virtio/virtio_balloon.c
> >> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >> >>>
> >> >>>       available = si_mem_available();
> >> >>>
> >> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> >> >>>                               pages_to_bytes(events[PSWPIN]));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> >> >>>                               pages_to_bytes(events[PSWPOUT]));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> >> >>> +#endif
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
> >> >>>                               pages_to_bytes(i.freeram));
> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> >> >
> >> > This will leave four uninitialized slots in vb->stats if
> >> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
> >> > have
> >> >
> >> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
> >> >
> >> > at the end.
> >> >
> >> > You need to make sure that vb->stats is smaller, either by using
> >> > something else than VIRTIO_BALLOON_S_NR for its size or something else
> >> > than sizeof(vb->stats) as the last argument to sg_init_one in this
> >> > file.
> >>
> >> Ah, got it. Would one of you create a fixed patch for this, or should I?
> 
> I will do it. If we go the route of returning 'idx' from
> update_balloon_stats, my patch
> 
>    virtio_balloon: don't push uninitialized buffers to stats virtqueue
> 
> needs to be redone anyway. Thanks!

Can you do it soon pls? I'd like to do a pull request.

> >> An easy way to solve it would be to preinitialize the events array
> >> and return zeroes to the host, but I don't know if that's the right
> >> solution.
> >
> >
> > I think that's out of spec.
> >
> >> Another option would be to return 'idx' from update_balloon_stats,
> >> and use that for the size:
> >>
> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> >> index cd5c54e2003d..bea3cfb88e53 100644
> >> --- a/drivers/virtio/virtio_balloon.c
> >> +++ b/drivers/virtio/virtio_balloon.c
> >> @@ -242,7 +242,7 @@ static inline void update_stat(struct
> >> virtio_balloon *vb, int idx,
> >>
> >>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> >>
> >> -static void update_balloon_stats(struct virtio_balloon *vb)
> >> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> >>  {
> >>   unsigned long events[NR_VM_EVENT_ITEMS];
> >>   struct sysinfo i;
> >> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
> >>   pages_to_bytes(i.totalram));
> >>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> >>   pages_to_bytes(available));
> >> +
> >> + return idx;
> >>  }
> >>
> >>  /*
> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
> >> virtio_balloon *vb)
> >>   struct virtqueue *vq;
> >>   struct scatterlist sg;
> >>   unsigned int len;
> >> + unsigned int num_stats;
> >>
> >> - update_balloon_stats(vb);
> >> + num_stats = update_balloon_stats(vb);
> >>
> >>   vq = vb->stats_vq;
> >>   if (!virtqueue_get_buf(vq, &len))
> >>   return;
> >> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> >> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
> >>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> >>   virtqueue_kick(vq);
> >>  }
> >>
> >>      Arnd
> >
> > Sounds reasonable.
> >
> > --
> > MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
  2017-03-28 16:18             ` Michael S. Tsirkin
@ 2017-03-28 16:48               ` Ladi Prosek
  -1 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-28 16:48 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, David Hildenbrand, Jason Wang, Yisheng Xie,
	Konstantin Neumoin, Linux Kernel Mailing List, virtualization,
	Minchan Kim, Denis V. Lunev, Andrew Morton, Ingo Molnar

On Tue, Mar 28, 2017 at 6:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 27, 2017 at 12:02:33PM +0200, Ladi Prosek wrote:
>> On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> >> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> >> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >> >>>
>> >> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>>
>> >> >>> This seems absolutely right, so we should add an extra check to
>> >> >>> prevent copying uninitialized stack data into the statistics.
>> >> >>> From all I can tell, this has been broken since the statistics code
>> >> >>> was originally added in 2.6.34.
>> >> >>>
>> >> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> >> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >> >>> ---
>> >> >>>  drivers/virtio/virtio_balloon.c | 2 ++
>> >> >>>  1 file changed, 2 insertions(+)
>> >> >>>
>> >> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >> >>> index 4e1191508228..cd5c54e2003d 100644
>> >> >>> --- a/drivers/virtio/virtio_balloon.c
>> >> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >> >>>
>> >> >>>       available = si_mem_available();
>> >> >>>
>> >> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >> >>>                               pages_to_bytes(events[PSWPIN]));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >> >>>                               pages_to_bytes(events[PSWPOUT]));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> >> >>> +#endif
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >> >>>                               pages_to_bytes(i.freeram));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >> >
>> >> > This will leave four uninitialized slots in vb->stats if
>> >> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> >> > have
>> >> >
>> >> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >> >
>> >> > at the end.
>> >> >
>> >> > You need to make sure that vb->stats is smaller, either by using
>> >> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> >> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> >> > file.
>> >>
>> >> Ah, got it. Would one of you create a fixed patch for this, or should I?
>>
>> I will do it. If we go the route of returning 'idx' from
>> update_balloon_stats, my patch
>>
>>    virtio_balloon: don't push uninitialized buffers to stats virtqueue
>>
>> needs to be redone anyway. Thanks!
>
> Can you do it soon pls? I'd like to do a pull request.

Done.

   [PATCH v3 0/3] virtio_balloon: don't push uninitialized buffers to
stats virtqueue

Thanks!

>> >> An easy way to solve it would be to preinitialize the events array
>> >> and return zeroes to the host, but I don't know if that's the right
>> >> solution.
>> >
>> >
>> > I think that's out of spec.
>> >
>> >> Another option would be to return 'idx' from update_balloon_stats,
>> >> and use that for the size:
>> >>
>> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >> index cd5c54e2003d..bea3cfb88e53 100644
>> >> --- a/drivers/virtio/virtio_balloon.c
>> >> +++ b/drivers/virtio/virtio_balloon.c
>> >> @@ -242,7 +242,7 @@ static inline void update_stat(struct
>> >> virtio_balloon *vb, int idx,
>> >>
>> >>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>> >>
>> >> -static void update_balloon_stats(struct virtio_balloon *vb)
>> >> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>> >>  {
>> >>   unsigned long events[NR_VM_EVENT_ITEMS];
>> >>   struct sysinfo i;
>> >> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >>   pages_to_bytes(i.totalram));
>> >>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>> >>   pages_to_bytes(available));
>> >> +
>> >> + return idx;
>> >>  }
>> >>
>> >>  /*
>> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> >> virtio_balloon *vb)
>> >>   struct virtqueue *vq;
>> >>   struct scatterlist sg;
>> >>   unsigned int len;
>> >> + unsigned int num_stats;
>> >>
>> >> - update_balloon_stats(vb);
>> >> + num_stats = update_balloon_stats(vb);
>> >>
>> >>   vq = vb->stats_vq;
>> >>   if (!virtqueue_get_buf(vq, &len))
>> >>   return;
>> >> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>> >> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>> >>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>> >>   virtqueue_kick(vq);
>> >>  }
>> >>
>> >>      Arnd
>> >
>> > Sounds reasonable.
>> >
>> > --
>> > MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use
@ 2017-03-28 16:48               ` Ladi Prosek
  0 siblings, 0 replies; 18+ messages in thread
From: Ladi Prosek @ 2017-03-28 16:48 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Yisheng Xie, Arnd Bergmann, Konstantin Neumoin,
	Linux Kernel Mailing List, virtualization, Minchan Kim,
	Denis V. Lunev, Andrew Morton, Ingo Molnar

On Tue, Mar 28, 2017 at 6:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 27, 2017 at 12:02:33PM +0200, Ladi Prosek wrote:
>> On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote:
>> >> On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david@redhat.com> wrote:
>> >> >> On 23.03.2017 16:17, Arnd Bergmann wrote:
>> >> >>> The latest gcc-7.0.1 snapshot reports a new warning:
>> >> >>>
>> >> >>> virtio/virtio_balloon.c: In function 'update_balloon_stats':
>> >> >>> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
>> >> >>>
>> >> >>> This seems absolutely right, so we should add an extra check to
>> >> >>> prevent copying uninitialized stack data into the statistics.
>> >> >>> From all I can tell, this has been broken since the statistics code
>> >> >>> was originally added in 2.6.34.
>> >> >>>
>> >> >>> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
>> >> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >> >>> ---
>> >> >>>  drivers/virtio/virtio_balloon.c | 2 ++
>> >> >>>  1 file changed, 2 insertions(+)
>> >> >>>
>> >> >>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >> >>> index 4e1191508228..cd5c54e2003d 100644
>> >> >>> --- a/drivers/virtio/virtio_balloon.c
>> >> >>> +++ b/drivers/virtio/virtio_balloon.c
>> >> >>> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >> >>>
>> >> >>>       available = si_mem_available();
>> >> >>>
>> >> >>> +#ifdef CONFIG_VM_EVENT_COUNTERS
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>> >> >>>                               pages_to_bytes(events[PSWPIN]));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>> >> >>>                               pages_to_bytes(events[PSWPOUT]));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
>> >> >>> +#endif
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>> >> >>>                               pages_to_bytes(i.freeram));
>> >> >>>       update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
>> >> >
>> >> > This will leave four uninitialized slots in vb->stats if
>> >> > CONFIG_VM_EVENT_COUNTERS is not defined. update_balloon_stats should
>> >> > have
>> >> >
>> >> >   BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>> >> >
>> >> > at the end.
>> >> >
>> >> > You need to make sure that vb->stats is smaller, either by using
>> >> > something else than VIRTIO_BALLOON_S_NR for its size or something else
>> >> > than sizeof(vb->stats) as the last argument to sg_init_one in this
>> >> > file.
>> >>
>> >> Ah, got it. Would one of you create a fixed patch for this, or should I?
>>
>> I will do it. If we go the route of returning 'idx' from
>> update_balloon_stats, my patch
>>
>>    virtio_balloon: don't push uninitialized buffers to stats virtqueue
>>
>> needs to be redone anyway. Thanks!
>
> Can you do it soon pls? I'd like to do a pull request.

Done.

   [PATCH v3 0/3] virtio_balloon: don't push uninitialized buffers to
stats virtqueue

Thanks!

>> >> An easy way to solve it would be to preinitialize the events array
>> >> and return zeroes to the host, but I don't know if that's the right
>> >> solution.
>> >
>> >
>> > I think that's out of spec.
>> >
>> >> Another option would be to return 'idx' from update_balloon_stats,
>> >> and use that for the size:
>> >>
>> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> >> index cd5c54e2003d..bea3cfb88e53 100644
>> >> --- a/drivers/virtio/virtio_balloon.c
>> >> +++ b/drivers/virtio/virtio_balloon.c
>> >> @@ -242,7 +242,7 @@ static inline void update_stat(struct
>> >> virtio_balloon *vb, int idx,
>> >>
>> >>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>> >>
>> >> -static void update_balloon_stats(struct virtio_balloon *vb)
>> >> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
>> >>  {
>> >>   unsigned long events[NR_VM_EVENT_ITEMS];
>> >>   struct sysinfo i;
>> >> @@ -268,6 +268,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>> >>   pages_to_bytes(i.totalram));
>> >>   update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>> >>   pages_to_bytes(available));
>> >> +
>> >> + return idx;
>> >>  }
>> >>
>> >>  /*
>> >> @@ -294,13 +296,14 @@ static void stats_handle_request(struct
>> >> virtio_balloon *vb)
>> >>   struct virtqueue *vq;
>> >>   struct scatterlist sg;
>> >>   unsigned int len;
>> >> + unsigned int num_stats;
>> >>
>> >> - update_balloon_stats(vb);
>> >> + num_stats = update_balloon_stats(vb);
>> >>
>> >>   vq = vb->stats_vq;
>> >>   if (!virtqueue_get_buf(vq, &len))
>> >>   return;
>> >> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>> >> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0] * num_stats));
>> >>   virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
>> >>   virtqueue_kick(vq);
>> >>  }
>> >>
>> >>      Arnd
>> >
>> > Sounds reasonable.
>> >
>> > --
>> > MST

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-03-28 16:57 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-23 15:17 [PATCH] virtio_balloon: prevent uninitialized variable use Arnd Bergmann
2017-03-23 15:17 ` Arnd Bergmann
2017-03-23 15:31 ` Denis V. Lunev
2017-03-23 15:31   ` Denis V. Lunev
2017-03-24 18:38 ` David Hildenbrand
2017-03-24 18:38   ` David Hildenbrand
2017-03-24 20:11   ` Ladi Prosek
2017-03-24 20:11     ` Ladi Prosek
2017-03-24 20:40     ` Arnd Bergmann
2017-03-24 20:40       ` Arnd Bergmann
2017-03-24 20:59       ` Michael S. Tsirkin
2017-03-24 20:59         ` Michael S. Tsirkin
2017-03-27 10:02         ` Ladi Prosek
2017-03-27 10:02           ` Ladi Prosek
2017-03-28 16:18           ` Michael S. Tsirkin
2017-03-28 16:18             ` Michael S. Tsirkin
2017-03-28 16:48             ` Ladi Prosek
2017-03-28 16:48               ` Ladi Prosek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.