From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755430AbdC1QTN (ORCPT ); Tue, 28 Mar 2017 12:19:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48636 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752402AbdC1QTL (ORCPT ); Tue, 28 Mar 2017 12:19:11 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B346F67BC9 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=mst@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B346F67BC9 Date: Tue, 28 Mar 2017 19:18:58 +0300 From: "Michael S. Tsirkin" To: Ladi Prosek Cc: Arnd Bergmann , David Hildenbrand , Jason Wang , Yisheng Xie , Konstantin Neumoin , Linux Kernel Mailing List , virtualization@lists.linux-foundation.org, Minchan Kim , "Denis V. Lunev" , Andrew Morton , Ingo Molnar Subject: Re: [PATCH] virtio_balloon: prevent uninitialized variable use Message-ID: <20170328191839-mutt-send-email-mst@kernel.org> References: <20170323151728.679684-1-arnd@arndb.de> <20170324235900-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 28 Mar 2017 16:19:05 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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); > >> > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio_balloon: prevent uninitialized variable use Date: Tue, 28 Mar 2017 19:18:58 +0300 Message-ID: <20170328191839-mutt-send-email-mst@kernel.org> References: <20170323151728.679684-1-arnd@arndb.de> <20170324235900-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline 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: Ladi Prosek Cc: Yisheng Xie , Arnd Bergmann , Konstantin Neumoin , Linux Kernel Mailing List , virtualization@lists.linux-foundation.org, Minchan Kim , "Denis V. Lunev" , Andrew Morton , Ingo Molnar List-Id: virtualization@lists.linuxfoundation.org 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); > >> > >> 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