From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755322AbbG0Bga (ORCPT ); Sun, 26 Jul 2015 21:36:30 -0400 Received: from mx2.suse.de ([195.135.220.15]:39454 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751431AbbG0Bg2 (ORCPT ); Sun, 26 Jul 2015 21:36:28 -0400 Date: Mon, 27 Jul 2015 11:36:21 +1000 From: NeilBrown To: Benjamin Randazzo Cc: linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drivers/md/md.c: use kzalloc() when bitmap is disabled Message-ID: <20150727113621.7eef4eec@noble> In-Reply-To: <1437835010-11430-1-git-send-email-benjamin@randazzo.fr> References: <1437835010-11430-1-git-send-email-benjamin@randazzo.fr> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.28; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 25 Jul 2015 16:36:50 +0200 Benjamin Randazzo wrote: > In drivers/md/md.c get_bitmap_file() uses kmalloc() for creating a > mdu_bitmap_file_t called "file". > > 5769 file = kmalloc(sizeof(*file), GFP_NOIO); > 5770 if (!file) > 5771 return -ENOMEM; > > This structure is copied to user space at the end of the function. > > 5786 if (err == 0 && > 5787 copy_to_user(arg, file, sizeof(*file))) > 5788 err = -EFAULT > > But if bitmap is disabled only the first byte of "file" is initialized > with zero, so it's possible to read some bytes (up to 4095) of kernel > space memory from user space. This is an information leak. > > 5775 /* bitmap disabled, zero the first byte and copy out */ > 5776 if (!mddev->bitmap_info.file) > 5777 file->pathname[0] = '\0'; > > Signed-off-by: Benjamin Randazzo > --- > drivers/md/md.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 80879dc..382bdbc 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -5766,22 +5766,21 @@ static int get_bitmap_file(struct mddev *mddev, void __user * arg) > char *ptr; > int err; > > - file = kmalloc(sizeof(*file), GFP_NOIO); > + file = kzalloc(sizeof(*file), GFP_NOIO); > if (!file) > return -ENOMEM; > > err = 0; > spin_lock(&mddev->lock); > - /* bitmap disabled, zero the first byte and copy out */ > - if (!mddev->bitmap_info.file) > - file->pathname[0] = '\0'; > - else if ((ptr = file_path(mddev->bitmap_info.file, > - file->pathname, sizeof(file->pathname))), > - IS_ERR(ptr)) > - err = PTR_ERR(ptr); > - else > - memmove(file->pathname, ptr, > - sizeof(file->pathname)-(ptr-file->pathname)); > + /* bitmap enabled */ > + if (mddev->bitmap_info.file) { > + if ((ptr = file_path(mddev->bitmap_info.file, file->pathname, > + sizeof(file->pathname))), IS_ERR(ptr)) > + err = PTR_ERR(ptr); > + else > + memmove(file->pathname, ptr, > + sizeof(file->pathname)-(ptr-file->pathname)); > + } > spin_unlock(&mddev->lock); > > if (err == 0 && Thanks. I re-arranged the code a little bit more as there is no longer any excuse for having the "ptr = file_path()" assignment inside the condition of the 'if'. Applied. Thanks, NeilBrown