From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH 3/7] kvmtool: fix strcpy vulnerabilities Date: Thu, 17 Nov 2016 15:30:07 +0000 Message-ID: <20161117153007.GK22855@arm.com> References: <1476806572-9782-1-git-send-email-gcampana+kvm@quarkslab.com> <20161108020847.GV20591@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, andre.przywara@arm.com To: "G. Campana" Return-path: Received: from foss.arm.com ([217.140.101.70]:57900 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754784AbcKQR5V (ORCPT ); Thu, 17 Nov 2016 12:57:21 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 10, 2016 at 04:15:12PM +0100, G. Campana wrote: > On 08/11/2016 03:08, Will Deacon wrote: > > On Tue, Oct 18, 2016 at 06:02:52PM +0200, G. Campana wrote: > >> --- > >> virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++----------------- > >> 1 file changed, 43 insertions(+), 17 deletions(-) > >> > >> diff --git a/virtio/9p.c b/virtio/9p.c > >> index 9695540..cd93d06 100644 > >> --- a/virtio/9p.c > >> +++ b/virtio/9p.c > >> @@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid) > >> { > >> struct rb_node *node = dev->fids.rb_node; > >> struct p9_fid *pfid = NULL; > >> + size_t len; > >> > >> while (node) { > >> struct p9_fid *cur = rb_entry(node, struct p9_fid, node); > >> @@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid) > >> if (!pfid) > >> return NULL; > >> > >> + len = strlen(dev->root_dir); > >> + if (len >= sizeof(pfid->path)) { > >> + free(pfid); > >> + return NULL; > >> + } > > > > This doesn't make sense to me -- pfid->path is just a NULL ptr at this > > stage. Did you mean abs_path? > > > Good catch. Indeed, I meant abs_path. > > >> + > >> pfid->fid = fid; > >> - strcpy(pfid->abs_path, dev->root_dir); > >> - pfid->path = pfid->abs_path + strlen(dev->root_dir); > >> + strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path)); > > > > But if you did mean abs_path, why use strncpy here? > > > It ensures that the string is null terminated. Given that we already did a strlen on root_dir, and we know that abs_path is big enough to hold it, I still think this isn't needed. Will