From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753278AbdFWOSw (ORCPT ); Fri, 23 Jun 2017 10:18:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:37975 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750932AbdFWOSv (ORCPT ); Fri, 23 Jun 2017 10:18:51 -0400 Date: Fri, 23 Jun 2017 16:18:37 +0200 From: Michal Hocko To: Kees Cook Cc: Andrew Morton , Alexander Viro , Qualys Security Advisory , Linux-MM , "linux-fsdevel@vger.kernel.org" , LKML , "kernel-hardening@lists.openwall.com" Subject: Re: [PATCH] exec: Account for argv/envp pointers Message-ID: <20170623141837.GD5308@dhcp22.suse.cz> References: <20170622001720.GA32173@beast> <20170623135924.GC5314@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko Thanks! -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 23 Jun 2017 16:18:37 +0200 From: Michal Hocko To: Kees Cook Cc: Andrew Morton , Alexander Viro , Qualys Security Advisory , Linux-MM , "linux-fsdevel@vger.kernel.org" , LKML , "kernel-hardening@lists.openwall.com" Subject: Re: [PATCH] exec: Account for argv/envp pointers Message-ID: <20170623141837.GD5308@dhcp22.suse.cz> References: <20170622001720.GA32173@beast> <20170623135924.GC5314@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko Thanks! -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 23 Jun 2017 16:18:37 +0200 From: Michal Hocko Message-ID: <20170623141837.GD5308@dhcp22.suse.cz> References: <20170622001720.GA32173@beast> <20170623135924.GC5314@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers To: Kees Cook Cc: Andrew Morton , Alexander Viro , Qualys Security Advisory , Linux-MM , "linux-fsdevel@vger.kernel.org" , LKML , "kernel-hardening@lists.openwall.com" List-ID: On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko Thanks! -- Michal Hocko SUSE Labs