Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache

From	"Darrick J. Wong" <darrick.wong@oracle.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache
Date	2017-08-29 21:10 +0200
Message-ID	<ujU3D-6AB-13@gated-at.bofh.it> (permalink)
References	(1 earlier) <ujzVg-2ie-37@gated-at.bofh.it> <ujAeB-2oT-13@gated-at.bofh.it> <ujAeB-2oT-11@gated-at.bofh.it> <ujGDn-6rm-1@gated-at.bofh.it> <ujTKh-6eS-1@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Tue, Aug 29, 2017 at 11:48:49AM -0700, Kees Cook wrote:
> On Mon, Aug 28, 2017 at 9:47 PM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > On Mon, Aug 28, 2017 at 02:57:14PM -0700, Kees Cook wrote:
> >> On Mon, Aug 28, 2017 at 2:49 PM, Darrick J. Wong
> >> <darrick.wong@oracle.com> wrote:
> >> > On Mon, Aug 28, 2017 at 02:34:56PM -0700, Kees Cook wrote:
> >> >> From: David Windsor <dave@nullcore.net>
> >> >>
> >> >> The XFS inline inode data, stored in struct xfs_inode_t field
> >> >> i_df.if_u2.if_inline_data and therefore contained in the xfs_inode slab
> >> >> cache, needs to be copied to/from userspace.
> >> >>
> >> >> cache object allocation:
> >> >>     fs/xfs/xfs_icache.c:
> >> >>         xfs_inode_alloc(...):
> >> >>             ...
> >> >>             ip = kmem_zone_alloc(xfs_inode_zone, KM_SLEEP);
> >> >>
> >> >>     fs/xfs/libxfs/xfs_inode_fork.c:
> >> >>         xfs_init_local_fork(...):
> >> >>             ...
> >> >>             if (mem_size <= sizeof(ifp->if_u2.if_inline_data))
> >> >>                     ifp->if_u1.if_data = ifp->if_u2.if_inline_data;
> >> >
> >> > Hmm, what happens when mem_size > sizeof(if_inline_data)?  A slab object
> >> > will be allocated for ifp->if_u1.if_data which can then be used for
> >> > readlink in the same manner as the example usage trace below.  Does
> >> > that allocated object have a need for a usercopy annotation like
> >> > the one we're adding for if_inline_data?  Or is that already covered
> >> > elsewhere?
> >>
> >> Yeah, the xfs helper kmem_alloc() is used in the other case, which
> >> ultimately boils down to a call to kmalloc(), which is entirely
> >> whitelisted by an earlier patch in the series:
> >>
> >> https://lkml.org/lkml/2017/8/28/1026
> >
> > Ah.  It would've been helpful to have the first three patches cc'd to
> > the xfs list.  So basically this series establishes the ability to set
> 
> I went back and forth on that, and given all the things it touched, it
> seemed like too large a CC list. :) I can explicitly add the xfs list
> to the first three for any future versions.
> 
> > regions within a slab object into which copy_to_user can copy memory
> > contents, and vice versa.  Have you seen any runtime performance impact?
> > The overhead looks like it ought to be minimal.
> 
> Under CONFIG_HARDENED_USERCOPY, there's no difference in performance
> between the earlier bounds checking (of the whole slab object) vs the
> new bounds checking (of the useroffset/usersize portion of the slab
> object). Perf difference of CONFIG_HARDENED_USERCOPY itself has proven
> hard to measure, which likely means it's very minimal.
> 
> >> (It's possible that at some future time we can start segregating
> >> kernel-only kmallocs from usercopy-able kmallocs, but for now, there
> >> are no plans for this.)
> >
> > A pity.  It would be interesting to create no-usercopy versions of the
> > kmalloc-* slabs and see how much of XFS' memory consumption never
> > touches userspace buffers. :)
> 
> There are plans for building either a new helper (kmalloc_usercopy())
> or adding a new flag (GFP_USERCOPY), but I haven't had time yet to
> come back around to it. I wanted to land this step first, and we could
> then move forward on the rest in future.

Heh, fair enough.

For the XFS bits,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thread

[PATCH v2 00/30] Hardened usercopy whitelisting Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 07/30] ext4: Define usercopy region in ext4_inode_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 09/30] jfs: Define usercopy region in jfs_ip slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 06/30] vfs: Copy struct mount.mnt_id to userspace using put_user() Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 05/30] vfs: Define usercopy region in names_cache slab caches Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 08/30] ext2: Define usercopy region in ext2_inode_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
    Re: [PATCH v2 08/30] ext2: Define usercopy region in  ext2_inode_cache slab cache Jan Kara <jack@suse.cz> - 2017-08-30 13:30 +0200
  [PATCH v2 13/30] ufs: Define usercopy region in ufs_inode_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 16/30] cifs: Define usercopy region in cifs_request slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 02/30] usercopy: Enforce slab cache usercopy region boundaries Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 20/30] caif: Define usercopy region in caif proto slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 01/30] usercopy: Prepare for usercopy whitelisting Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 11/30] exofs: Define usercopy region in exofs_inode_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 04/30] dcache: Define usercopy region in dentry_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
  [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
    Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-29 00:00 +0200
      Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-29 06:50 +0200
        Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-29 20:50 +0200
          Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-29 21:10 +0200
          Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Dave Chinner <david@fromorbit.com> - 2017-08-30 00:20 +0200
            Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-30 00:30 +0200
    Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-08-29 00:00 +0200
    Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Christoph Hellwig <hch@infradead.org> - 2017-08-29 10:20 +0200
      Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Dave Chinner <david@fromorbit.com> - 2017-08-29 14:40 +0200
        Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Christoph Hellwig <hch@infradead.org> - 2017-08-29 14:50 +0200
          Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Dave Chinner <david@fromorbit.com> - 2017-08-30 00:00 +0200
            Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Christoph Hellwig <hch@infradead.org> - 2017-08-30 09:20 +0200
              Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Dave Chinner <david@fromorbit.com> - 2017-08-30 10:10 +0200
                Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab  cache Christoph Hellwig <hch@infradead.org> - 2017-08-30 10:40 +0200
      Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-29 21:00 +0200
  [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:40 +0200
    Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache  slab cache Luis de Bethencourt <luisbg@kernel.org> - 2017-08-29 12:20 +0200
      Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache  slab cache Kees Cook <keescook@chromium.org> - 2017-08-29 17:40 +0200
        Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache  slab cache Luis de Bethencourt <luisbg@kernel.org> - 2017-08-29 19:20 +0200
  [PATCH v2 25/30] fork: Define usercopy region in thread_stack slab caches Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
    Re: [kernel-hardening] [PATCH v2 25/30] fork: Define usercopy  region in thread_stack slab caches Rik van Riel <riel@redhat.com> - 2017-08-30 21:00 +0200
  [PATCH v2 21/30] sctp: Define usercopy region in SCTP proto slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 19/30] ip: Define usercopy region in IP proto slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 22/30] sctp: Copy struct sctp_sock.autoclose to userspace using put_user() Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 29/30] arm: Implement thread_struct whitelist for hardened usercopy Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 30/30] usercopy: Restrict non-usercopy caches to size 0 Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 14/30] vxfs: Define usercopy region in vxfs_inode slab cache Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 24/30] fork: Define usercopy region in mm_struct slab caches Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
    Re: [kernel-hardening] [PATCH v2 24/30] fork: Define usercopy  region in mm_struct slab caches Rik van Riel <riel@redhat.com> - 2017-08-30 21:30 +0200
  [PATCH v2 23/30] net: Restrict unwhitelisted proto caches to size 0 Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 26/30] fork: Provide usercopy whitelisting for task_struct Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
    Re: [kernel-hardening] [PATCH v2 26/30] fork: Provide usercopy  whitelisting for task_struct Rik van Riel <riel@redhat.com> - 2017-08-30 21:00 +0200
  [PATCH v2 28/30] arm64: Implement thread_struct whitelist for hardened usercopy Kees Cook <keescook@chromium.org> - 2017-08-28 23:50 +0200
  [PATCH v2 27/30] x86: Implement thread_struct whitelist for hardened usercopy Kees Cook <keescook@chromium.org> - 2017-08-29 00:00 +0200
    Re: [kernel-hardening] [PATCH v2 27/30] x86: Implement  thread_struct whitelist for hardened usercopy Rik van Riel <riel@redhat.com> - 2017-08-30 21:00 +0200
  [PATCH v2 18/30] net: Define usercopy region in struct proto slab cache Kees Cook <keescook@chromium.org> - 2017-08-29 00:00 +0200

csiph-web