oss-sec mailing list archives
Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size
From: Roland McGrath <roland () redhat com>
Date: Fri, 10 Sep 2010 01:59:15 -0700 (PDT)
+ if (unlikely(stack_top < mmap_min_addr) ||this could arguably elicit some warning, or even better, prevent the sysctl from setting mmap_min_addr too high in the first place.
This code has local checks to ensure that things don't fail worse later. Those are good to have regardless. If you'd like to add some constraints on setting mmap_min_addr, that's certainly fine too. But it's no reason not to have this simple and clear check here.
or alternatively, just check for mmap_min_addr > stack_top - (vma->vm_end - vma->vm_start)
IMHO that is far less clear as to what the intent of the check is than what I wrote. It's especially subtle that this check allows overflow and then you check for that later, rather than the formulation I gave where no overflow is possible and it's immediately obvious why.
which looks even better if done in shift_arg_pages as i've done it in PaX:
My change to setup_arg_pages is consistent with the existing CONFIG_STACK_GROWSUP code. IMHO simple fixes should go in first and other restructuring of the code can be done later.
+ return -ENOMEM;i'd say EFAULT is more consistent, ENOMEM is used for cases when the kernel couldn't allocate physical memory for something, EFAULT is when there's some issue with the address space itself (based on the reaction to find_vma or expand_stack failures).
I disagree. IMHO, the only proper use of EFAULT is for the passing of a bad pointer (including passing data that includes a bad pointer, etc.). So execve really should only use EFAULT when the pointers passed in the system call are bad. I used ENOMEM purely for the reason that the existing CONFIG_STACK_GROWSUP code I paralleled uses it. But it certainly does make more sense than EFAULT does. ENOMEM is used for cases where you couldn't get the address space you wanted (mmap), which is a fairly close analogue to this case. But the one error code that is actually correct for the case is E2BIG. Obviously that's really the right thing for Argument list too long cases. The choice of error code here is fairly academic. Any failure of setup_arg_pages always leads to sending ourselves a SIGKILL before returning the error code to percolate back to execve. So it's only the error code seen in syscall tracing before the process dies. It's not "user-visible" in the usual sense (i.e. POSIX doesn't care what code we use here).
also what about the BUG_ON in shift_arg_pages? it could go now, right?
It is now impossible by the logic of the arithmetic, yes. But it is another local sanity check asserting the assumptions of the code in that function. So there is no reason to
personally, i prefer to do this the way i did it in PaX: move up the shift_arg_pages call a bit and turn the BUG_ON into a proper check.
I'm more comfortable with a fix that doesn't change any other aspect of the behavior. Probably moving the call to shift_arg_pages around is fine. But IMHO that belongs in a separate set of cleanup patches, and not in the isolated fix for the BUG_ON hit. Thanks, Roland
Current thread:
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size, (continued)
- Message not available
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size KOSAKI Motohiro (Sep 09)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 10)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size KOSAKI Motohiro (Sep 10)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 11)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 15)
- Message not available
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 10)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 11)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 14)
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size pageexec (Sep 14)
- Message not available
- Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath (Sep 10)
- [PATCH 2/3] execve: improve interactivity with large arguments Roland McGrath (Sep 07)
- [PATCH 3/3] execve: make responsive to SIGKILL with large arguments Roland McGrath (Sep 07)
- Re: [PATCH 0/3] execve argument-copying fixes KOSAKI Motohiro (Sep 07)
- [PATCH 0/2] execve memory exhaust of argument-copying fixes KOSAKI Motohiro (Sep 09)
- [PATCH 1/2] oom: don't ignore rss in nascent mm KOSAKI Motohiro (Sep 09)
- Message not available
- Re: [PATCH 1/2] oom: don't ignore rss in nascent mm Roland McGrath (Sep 10)