| Message ID | 20211213142703.3066590-1-willy@infradead.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Assorted improvements to usercopy | expand |
I like these, but a quick question: Since the usercopy_abort() calls are all because the offset exceeds the page size, is there a reason why you don't specifically state that via the detail parameter rather than just supply a NULL pointer? Otherwise for the patch series: Reviewed-by: William Kucharski <william.kucharski@oracle.com> > On Dec 13, 2021, at 7:27 AM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > > We must prohibit page boundary crossing for kmap() addresses. > vmap() addresses are limited by the length of the mapping, and > compound pages are limited by the size of the page. > > These should probably all have test cases? > > v3: > - Remove a now-unused variable > v2: > - Prevent a NULL pointer dereference when a vmalloc-range pointer > doesn't have an associated allocation (me) > - Report better offsets than "0" (Kees) > > > Matthew Wilcox (Oracle) (3): > mm/usercopy: Check kmap addresses properly > mm/usercopy: Detect vmalloc overruns > mm/usercopy: Detect compound page overruns > > arch/x86/include/asm/highmem.h | 1 + > include/linux/highmem-internal.h | 10 ++++++++ > mm/usercopy.c | 43 +++++++++++++++++++++++--------- > 3 files changed, 42 insertions(+), 12 deletions(-) > > -- > 2.33.0
On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote: > I like these, but a quick question: > > Since the usercopy_abort() calls are all because the offset exceeds the page > size, is there a reason why you don't specifically state that via the detail > parameter rather than just supply a NULL pointer? Hmm ... I'd defer to Kees on this, because I'm not familiar with usercopy_abort() usage, but the only places which use the detail parameter today are slab/slub, which use it to pass the name of the slab. I think the user is supposed to infer that we overran the end of the page based on the offset & length values. > Otherwise for the patch series: > > Reviewed-by: William Kucharski <william.kucharski@oracle.com> Thanks!
On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote: > On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote: > > I like these, but a quick question: > > > > Since the usercopy_abort() calls are all because the offset exceeds the page > > size, is there a reason why you don't specifically state that via the detail > > parameter rather than just supply a NULL pointer? > > Hmm ... I'd defer to Kees on this, because I'm not familiar with > usercopy_abort() usage, but the only places which use the detail > parameter today are slab/slub, which use it to pass the name of > the slab. I think the user is supposed to infer that we overran the > end of the page based on the offset & length values. I agree that leaving it NULL is best here. The "detail" is really about adding more information about which thing it was, which for slab makes sense, but most other stuff there isn't really anything to quickly distinguish one from another (i.e. vmap is all vmap).
Thanks, good explanation. > On Dec 13, 2021, at 1:47 PM, Kees Cook <keescook@chromium.org> wrote: > > On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote: >> On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote: >>> I like these, but a quick question: >>> >>> Since the usercopy_abort() calls are all because the offset exceeds the page >>> size, is there a reason why you don't specifically state that via the detail >>> parameter rather than just supply a NULL pointer? >> >> Hmm ... I'd defer to Kees on this, because I'm not familiar with >> usercopy_abort() usage, but the only places which use the detail >> parameter today are slab/slub, which use it to pass the name of >> the slab. I think the user is supposed to infer that we overran the >> end of the page based on the offset & length values. > > I agree that leaving it NULL is best here. The "detail" is really about > adding more information about which thing it was, which for slab makes > sense, but most other stuff there isn't really anything to quickly > distinguish one from another (i.e. vmap is all vmap). > > -- > Kees Cook
On Mon, Dec 13, 2021 at 12:47:58PM -0800, Kees Cook wrote: > On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote: > > On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote: > > > I like these, but a quick question: > > > > > > Since the usercopy_abort() calls are all because the offset exceeds the page > > > size, is there a reason why you don't specifically state that via the detail > > > parameter rather than just supply a NULL pointer? > > > > Hmm ... I'd defer to Kees on this, because I'm not familiar with > > usercopy_abort() usage, but the only places which use the detail > > parameter today are slab/slub, which use it to pass the name of > > the slab. I think the user is supposed to infer that we overran the > > end of the page based on the offset & length values. > > I agree that leaving it NULL is best here. The "detail" is really about > adding more information about which thing it was, which for slab makes > sense, but most other stuff there isn't really anything to quickly > distinguish one from another (i.e. vmap is all vmap). There _is_ a bit more information in the vmap case (not in the kmap or compound page case). You can see it in /proc/vmallocinfo. We could pass it in like this? if (is_vmalloc_addr(ptr)) { struct vm_struct *vm = find_vm_area(ptr); + char sym[100]; unsigned long offset; if (!vm) { .. + if (vm->caller) + snprintf(sym, sizeof(sym), "%pS", vm->caller); offset = ptr - vm->addr; if (offset + n > vm->size) - usercopy_abort("vmalloc", NULL, to_user, offset, n); + usercopy_abort("vmalloc", vm->caller ? sym : NULL, + to_user, offset, n); return;
On Mon, Dec 13, 2021 at 09:16:46PM +0000, Matthew Wilcox wrote: > On Mon, Dec 13, 2021 at 12:47:58PM -0800, Kees Cook wrote: > > On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote: > > > On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote: > > > > I like these, but a quick question: > > > > > > > > Since the usercopy_abort() calls are all because the offset exceeds the page > > > > size, is there a reason why you don't specifically state that via the detail > > > > parameter rather than just supply a NULL pointer? > > > > > > Hmm ... I'd defer to Kees on this, because I'm not familiar with > > > usercopy_abort() usage, but the only places which use the detail > > > parameter today are slab/slub, which use it to pass the name of > > > the slab. I think the user is supposed to infer that we overran the > > > end of the page based on the offset & length values. > > > > I agree that leaving it NULL is best here. The "detail" is really about > > adding more information about which thing it was, which for slab makes > > sense, but most other stuff there isn't really anything to quickly > > distinguish one from another (i.e. vmap is all vmap). > > There _is_ a bit more information in the vmap case (not in the kmap > or compound page case). You can see it in /proc/vmallocinfo. We > could pass it in like this? > > if (is_vmalloc_addr(ptr)) { > struct vm_struct *vm = find_vm_area(ptr); > + char sym[100]; > unsigned long offset; > > if (!vm) { > .. > + if (vm->caller) > + snprintf(sym, sizeof(sym), "%pS", vm->caller); > offset = ptr - vm->addr; > if (offset + n > vm->size) > - usercopy_abort("vmalloc", NULL, to_user, offset, n); > + usercopy_abort("vmalloc", vm->caller ? sym : NULL, > + to_user, offset, n); That is interesting, but I think we don't want to do it here; adding to stack or making an allocation for this (even though it's slow-path) doesn't seem like a good idea as far as keeping code size down. -Kees > return; >