From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bYKG4-0001dH-NL for kexec@lists.infradead.org; Fri, 12 Aug 2016 21:44:53 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u7CLfjH7020796 for ; Fri, 12 Aug 2016 17:44:29 -0400 Received: from e24smtp02.br.ibm.com (e24smtp02.br.ibm.com [32.104.18.86]) by mx0a-001b2d01.pphosted.com with ESMTP id 24rmj8xj5c-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 12 Aug 2016 17:44:29 -0400 Received: from localhost by e24smtp02.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 12 Aug 2016 18:44:26 -0300 From: Thiago Jung Bauermann Subject: Re: [PATCH v2 2/2] kexec: extend kexec_file_load system call Date: Fri, 12 Aug 2016 18:44:16 -0300 In-Reply-To: <20160812081739.GB22218@350D> References: <1470956638-3589-1-git-send-email-bauerman@linux.vnet.ibm.com> <1470956638-3589-3-git-send-email-bauerman@linux.vnet.ibm.com> <20160812081739.GB22218@350D> MIME-Version: 1.0 Message-Id: <1606244.4vNzhVNUBR@hactar> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: bsingharora@gmail.com Cc: Stewart Smith , Mark Rutland , Arnd Bergmann , Baoquan He , Benjamin Herrenschmidt , Dave Young , kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Russell King - ARM Linux , AKASHI Takahiro , David Laight , Eric Biederman , Michael Ellerman , Andrew Morton , linuxppc-dev@lists.ozlabs.org, Vivek Goyal , linux-arm-kernel@lists.infradead.org Hello Balbir, Thank you for the review! Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh: > On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote: > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 3523bf62f328..847d9c31f428 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int); > > > > id(MODULE, kernel-module) \ > > id(KEXEC_IMAGE, kexec-image) \ > > id(KEXEC_INITRAMFS, kexec-initramfs) \ > > > > + id(KEXEC_PARTIAL_DTB, kexec-partial-dtb) \ > > The backspace is over-indented? Indeed, I'll fix that. But to keep it aligned with the other backslashes, there would be no spaces between it and the final closing parenthesis. Either that, or reindent the other backslashes one more level. I think I prefer the former. > > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, > > int kernel_fd, int initrd_fd,> > > image->initrd_buf_len = size; > > > > } > > > > + if (flags & KEXEC_FILE_EXTRA_FDS) { > > + int nr_fds, i; > > + size_t fdset_size; > > + char fdset_buf[MAX_FDSET_SIZE]; > > Do we really want this on the stack? I presume the size is not large It has 132 bytes. Would it be better to use kmalloc instead? > > + struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf; > > + > > + ret = copy_from_user(&nr_fds, ufdset, sizeof(int)); > > + if (ret) { > > + ret = -EFAULT; > > + goto out; > > + } > > + > > + if (nr_fds > KEXEC_SEGMENT_MAX) { > > We need an nr_fds < 0 check as well Indeed, I forgot to do that. I will add the check. > > + ret = -E2BIG; > > + goto out; > > + } > > + > > + fdset_size = sizeof(struct kexec_fdset) > > + + nr_fds * sizeof(struct kexec_file_fd); > > + > > + ret = copy_from_user(fdset, ufdset, fdset_size); > > Can the user change nr_fds between the two copy_from_users, ideally not, > but we should validate it. Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error if they're different. -- []'s Thiago Jung Bauermann IBM Linux Technology Center _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3s9z1T64Z0zDr2Q for ; Sat, 13 Aug 2016 07:44:29 +1000 (AEST) Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u7CLdYkS018018 for ; Fri, 12 Aug 2016 17:44:27 -0400 Received: from e24smtp03.br.ibm.com (e24smtp03.br.ibm.com [32.104.18.24]) by mx0b-001b2d01.pphosted.com with ESMTP id 24s2up7y1f-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 12 Aug 2016 17:44:27 -0400 Received: from localhost by e24smtp03.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 12 Aug 2016 18:44:25 -0300 Received: from d24relay01.br.ibm.com (d24relay01.br.ibm.com [9.8.31.16]) by d24dlp02.br.ibm.com (Postfix) with ESMTP id 2B6141DC0051 for ; Fri, 12 Aug 2016 17:44:14 -0400 (EDT) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.8.31.93]) by d24relay01.br.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u7CLiN3G4722854 for ; Fri, 12 Aug 2016 18:44:23 -0300 Received: from d24av02.br.ibm.com (localhost [127.0.0.1]) by d24av02.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u7CLiL6b004785 for ; Fri, 12 Aug 2016 18:44:23 -0300 From: Thiago Jung Bauermann To: bsingharora@gmail.com Cc: kexec@lists.infradead.org, Stewart Smith , Mark Rutland , Benjamin Herrenschmidt , Arnd Bergmann , Baoquan He , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Vivek Goyal , AKASHI Takahiro , David Laight , Eric Biederman , Michael Ellerman , Russell King - ARM Linux , Andrew Morton , Dave Young , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 2/2] kexec: extend kexec_file_load system call Date: Fri, 12 Aug 2016 18:44:16 -0300 In-Reply-To: <20160812081739.GB22218@350D> References: <1470956638-3589-1-git-send-email-bauerman@linux.vnet.ibm.com> <1470956638-3589-3-git-send-email-bauerman@linux.vnet.ibm.com> <20160812081739.GB22218@350D> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Message-Id: <1606244.4vNzhVNUBR@hactar> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hello Balbir, Thank you for the review! Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh: > On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote: > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 3523bf62f328..847d9c31f428 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int); > > > > id(MODULE, kernel-module) \ > > id(KEXEC_IMAGE, kexec-image) \ > > id(KEXEC_INITRAMFS, kexec-initramfs) \ > > > > + id(KEXEC_PARTIAL_DTB, kexec-partial-dtb) \ > > The backspace is over-indented? Indeed, I'll fix that. But to keep it aligned with the other backslashes, there would be no spaces between it and the final closing parenthesis. Either that, or reindent the other backslashes one more level. I think I prefer the former. > > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, > > int kernel_fd, int initrd_fd,> > > image->initrd_buf_len = size; > > > > } > > > > + if (flags & KEXEC_FILE_EXTRA_FDS) { > > + int nr_fds, i; > > + size_t fdset_size; > > + char fdset_buf[MAX_FDSET_SIZE]; > > Do we really want this on the stack? I presume the size is not large It has 132 bytes. Would it be better to use kmalloc instead? > > + struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf; > > + > > + ret = copy_from_user(&nr_fds, ufdset, sizeof(int)); > > + if (ret) { > > + ret = -EFAULT; > > + goto out; > > + } > > + > > + if (nr_fds > KEXEC_SEGMENT_MAX) { > > We need an nr_fds < 0 check as well Indeed, I forgot to do that. I will add the check. > > + ret = -E2BIG; > > + goto out; > > + } > > + > > + fdset_size = sizeof(struct kexec_fdset) > > + + nr_fds * sizeof(struct kexec_file_fd); > > + > > + ret = copy_from_user(fdset, ufdset, fdset_size); > > Can the user change nr_fds between the two copy_from_users, ideally not, > but we should validate it. Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error if they're different. -- []'s Thiago Jung Bauermann IBM Linux Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: bauerman@linux.vnet.ibm.com (Thiago Jung Bauermann) Date: Fri, 12 Aug 2016 18:44:16 -0300 Subject: [PATCH v2 2/2] kexec: extend kexec_file_load system call In-Reply-To: <20160812081739.GB22218@350D> References: <1470956638-3589-1-git-send-email-bauerman@linux.vnet.ibm.com> <1470956638-3589-3-git-send-email-bauerman@linux.vnet.ibm.com> <20160812081739.GB22218@350D> Message-ID: <1606244.4vNzhVNUBR@hactar> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Balbir, Thank you for the review! Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh: > On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote: > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 3523bf62f328..847d9c31f428 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int); > > > > id(MODULE, kernel-module) \ > > id(KEXEC_IMAGE, kexec-image) \ > > id(KEXEC_INITRAMFS, kexec-initramfs) \ > > > > + id(KEXEC_PARTIAL_DTB, kexec-partial-dtb) \ > > The backspace is over-indented? Indeed, I'll fix that. But to keep it aligned with the other backslashes, there would be no spaces between it and the final closing parenthesis. Either that, or reindent the other backslashes one more level. I think I prefer the former. > > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image, > > int kernel_fd, int initrd_fd,> > > image->initrd_buf_len = size; > > > > } > > > > + if (flags & KEXEC_FILE_EXTRA_FDS) { > > + int nr_fds, i; > > + size_t fdset_size; > > + char fdset_buf[MAX_FDSET_SIZE]; > > Do we really want this on the stack? I presume the size is not large It has 132 bytes. Would it be better to use kmalloc instead? > > + struct kexec_fdset *fdset = (struct kexec_fdset *) fdset_buf; > > + > > + ret = copy_from_user(&nr_fds, ufdset, sizeof(int)); > > + if (ret) { > > + ret = -EFAULT; > > + goto out; > > + } > > + > > + if (nr_fds > KEXEC_SEGMENT_MAX) { > > We need an nr_fds < 0 check as well Indeed, I forgot to do that. I will add the check. > > + ret = -E2BIG; > > + goto out; > > + } > > + > > + fdset_size = sizeof(struct kexec_fdset) > > + + nr_fds * sizeof(struct kexec_file_fd); > > + > > + ret = copy_from_user(fdset, ufdset, fdset_size); > > Can the user change nr_fds between the two copy_from_users, ideally not, > but we should validate it. Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error if they're different. -- []'s Thiago Jung Bauermann IBM Linux Technology Center