| Message ID | 20200608044121.stuEeVRhM%akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/54] mm/page_idle.c: skip offline pages | expand |
On Sun, Jun 7, 2020 at 9:41 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > One can set a lowest possible address in /proc/sys/vm/mmap_min_addr > and mmap below that bound nevertheless. Well, /proc/sys/vm/mmap_min_addr actually sets "dac_mmap_min_addr" directly, and then indirectly sets mmap_min_addr with slightly different rules. > It is possible to request a fixed mapping address below mmap_min_addr and > succeed. This update adds early checks of mmap_min_addr and mmap_end > boundaries and fixes the above issue. > > Apart from it is wrong I am not aware of any existing issue. Hmm. I think your patch is generally fine, although a few things worries me: - the "mmap_end" check seems wrong. It should be something like if (len > mmap_end || addr > mmap_end-len) shouldn't it? - the limit we _do_ test is that "dac_mmap_min_addr", and we allow lower limits for CAP_SYS_RAWIO - I think this will break vm86 mode on 32-bit x86. Have you tested that? In other words, I think the reason we don't do that hard check of mmap_min_addr is exactly that vm86 issue. If we were to force a hard check, we're now making it impossible to use vm86 mode. So I'm dropping this patch, because it is not clear that this was fully thought through. And if it *was* intentional, and people knew about the vm86 issues, and the thing about dac_mmap_min_addr and the check in cap_mmap_addr(), then it should be mentioned in the commit message. That said, I'd be more than willing to move the "cap_mmap_addr()" check into the mm layer and make it a whole lot more obvious. And I'd also be more than willing to really make the difference between "dac_mmap_min_addr" and "mmap_min_addr" actually meaningful, and really enforce "mmap_min_addr", because right now it's a half-baked feature that isn't actually used for anything but hinting and the grow-down issue. And that, in turn, is because of those vm86 issues, but also because we've historically had programs that wanted some legacy behavior and did a mmap() of a zero-page at the 0 address (because that's how some old Unix environments worked - I htink legacy parisc or similar). Linus
On Mon, Jun 8, 2020 at 10:50 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I'm dropping this patch, because it is not clear that this was > fully thought through. Side note: I'm also dropping 23/54, just because they go together, and because of how mmap_min_addr really isn't a hard limit (as things stand today). The logic in 23/54 fundamentally has that "it's a hard limit" issue. Linus
--- a/mm/mmap.c~mm-mmapc-do-not-allow-mappings-outside-of-allowed-limits +++ a/mm/mmap.c @@ -62,6 +62,14 @@ #define arch_mmap_check(addr, len, flags) (0) #endif +#ifndef arch_get_mmap_end +#define arch_get_mmap_end(addr) (TASK_SIZE) +#endif + +#ifndef arch_get_mmap_base +#define arch_get_mmap_base(addr, base) (base) +#endif + #ifdef CONFIG_HAVE_ARCH_MMAP_RND_BITS const int mmap_rnd_bits_min = CONFIG_ARCH_MMAP_RND_BITS_MIN; const int mmap_rnd_bits_max = CONFIG_ARCH_MMAP_RND_BITS_MAX; @@ -1369,6 +1377,7 @@ unsigned long do_mmap(struct file *file, unsigned long pgoff, unsigned long *populate, struct list_head *uf) { + const unsigned long mmap_end = arch_get_mmap_end(addr); struct mm_struct *mm = current->mm; int pkey = 0; @@ -1391,8 +1400,12 @@ unsigned long do_mmap(struct file *file, if (flags & MAP_FIXED_NOREPLACE) flags |= MAP_FIXED; - if (!(flags & MAP_FIXED)) + if (flags & MAP_FIXED) { + if ((addr < mmap_min_addr) || (addr > mmap_end)) + return -ENOMEM; + } else { addr = round_hint_to_min(addr); + } /* Careful about overflows.. */ len = PAGE_ALIGN(len); @@ -2074,14 +2087,6 @@ unsigned long vm_unmapped_area(struct vm return addr; } -#ifndef arch_get_mmap_end -#define arch_get_mmap_end(addr) (TASK_SIZE) -#endif - -#ifndef arch_get_mmap_base -#define arch_get_mmap_base(addr, base) (base) -#endif - /* Get an address range which is currently unmapped. * For shmat() with addr=0. *