* [PATCH] virtio_balloon: prevent uninitialized variable use @ 2017-03-23 15:17 Arnd Bergmann 2017-03-23 15:31 ` Denis V. Lunev 2017-03-24 18:38 ` David Hildenbrand 0 siblings, 2 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use 2017-03-23 15:17 [PATCH] virtio_balloon: prevent uninitialized variable use Arnd Bergmann @ 2017-03-23 15:31 ` Denis V. Lunev 2017-03-24 18:38 ` David Hildenbrand 1 sibling, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] virtio_balloon: prevent uninitialized variable use 2017-03-23 15:17 [PATCH] virtio_balloon: prevent uninitialized variable use Arnd Bergmann 2017-03-23 15:31 ` Denis V. Lunev @ 2017-03-24 18:38 ` David Hildenbrand 2017-03-24 20:11 ` Ladi Prosek 1 sibling, 1 reply; 9+ 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] 9+ 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 2017-03-24 20:40 ` Arnd Bergmann 0 siblings, 1 reply; 9+ 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] 9+ 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 2017-03-24 20:59 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ 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] 9+ 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 2017-03-27 10:02 ` Ladi Prosek 0 siblings, 1 reply; 9+ 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] 9+ 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 2017-03-28 16:18 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ 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] 9+ 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 2017-03-28 16:48 ` Ladi Prosek 0 siblings, 1 reply; 9+ 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] 9+ 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 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2017-03-28 16:57 UTC | newest] Thread overview: 9+ 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:31 ` Denis V. Lunev 2017-03-24 18:38 ` David Hildenbrand 2017-03-24 20:11 ` Ladi Prosek 2017-03-24 20:40 ` Arnd Bergmann 2017-03-24 20:59 ` Michael S. Tsirkin 2017-03-27 10:02 ` Ladi Prosek 2017-03-28 16:18 ` Michael S. Tsirkin 2017-03-28 16:48 ` Ladi Prosek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox