Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Tue, Mar 28, 2017 at 6:18 PM, Michael S. Tsirkin 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 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 wrote: >> >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand >> >> > 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 >> >> >>> --- >> >> >>> 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; >> >> } >> >> >> >> /* >>
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
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 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 wrote: > >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand > >> > 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 > >> >>> --- > >> >>> 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
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 9:59 PM, Michael S. Tsirkin 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 wrote: >> > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand >> > 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 >> >>> --- >> >>> 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
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 09:40:07PM +0100, Arnd Bergmann wrote: > On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek wrote: > > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand 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 > >>> --- > >>> 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 9:11 PM, Ladi Prosek wrote: > On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand 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 >>> --- >>> 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: prevent uninitialized variable use
On Fri, Mar 24, 2017 at 7:38 PM, David Hildenbrand 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 >> --- >> 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [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 > --- > 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [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 Reviewed-by: Denis V. Lunev > --- > 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, ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization