thr3ads.net - Virtualization - [PATCH] virtio_balloon: prevent uninitialized variable use [Mar 2017]

If this information is useful, please help other people find it:
Share via:

Arnd Bergmann

2017-Mar-23 15:17 UTC

[PATCH] virtio_balloon: prevent uninitialized variable use

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 codewas originally added in 2.6.34.

Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the
balloon driver (V4)")
Signed-off-by: Arnd Bergmann <arnd at 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

Denis V. Lunev

2017-Mar-23 15:31 UTC

head link

[PATCH] virtio_balloon: prevent uninitialized variable use

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 at arndb.de>Reviewed-by: Denis V. Lunev <den at 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,

David Hildenbrand

2017-Mar-24 18:38 UTC

head link

[PATCH] virtio_balloon: prevent uninitialized variable use

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 at 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

Ladi Prosek

2017-Mar-24 20:11 UTC

head link

[PATCH] virtio_balloon: prevent uninitialized variable use

On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand <david at 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 at 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

Possibly Parallel Threads

Search for more apparently analagous threads

Virtualization - Mar 2017 - [PATCH] virtio_balloon: prevent uninitialized variable use

[PATCH] virtio_balloon: prevent uninitialized variable use

[PATCH] virtio_balloon: prevent uninitialized variable use

[PATCH] virtio_balloon: prevent uninitialized variable use

[PATCH] virtio_balloon: prevent uninitialized variable use

Possibly Parallel Threads