From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967358AbdCXULf (ORCPT ); Fri, 24 Mar 2017 16:11:35 -0400 Received: from mail-vk0-f43.google.com ([209.85.213.43]:34276 "EHLO mail-vk0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751372AbdCXUL1 (ORCPT ); Fri, 24 Mar 2017 16:11:27 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170323151728.679684-1-arnd@arndb.de> From: Ladi Prosek Date: Fri, 24 Mar 2017 21:11:25 +0100 Message-ID: Subject: Re: [PATCH] virtio_balloon: prevent uninitialized variable use To: David Hildenbrand Cc: Arnd Bergmann , "Michael S. Tsirkin" , Jason Wang , Yisheng Xie , Konstantin Neumoin , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Minchan Kim , "Denis V. Lunev" , Andrew Morton , Ingo Molnar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ladi Prosek Subject: Re: [PATCH] virtio_balloon: prevent uninitialized variable use Date: Fri, 24 Mar 2017 21:11:25 +0100 Message-ID: References: <20170323151728.679684-1-arnd@arndb.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: David Hildenbrand Cc: Yisheng Xie , Arnd Bergmann , "Michael S. Tsirkin" , Konstantin Neumoin , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Minchan Kim , "Denis V. Lunev" , Andrew Morton , Ingo Molnar List-Id: virtualization@lists.linuxfoundation.org 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