Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1721983 > unrolled thread
| Started by | Kees Cook <keescook@chromium.org> |
|---|---|
| First post | 2017-08-28 23:40 +0200 |
| Last post | 2017-08-29 00:00 +0200 |
| Articles | 20 on this page of 51 — 7 participants |
Back to article view | Back to linux.kernel
[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
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-30 00:30 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujXbb-8u5-1@gated-at.bofh.it> |
| In reply to | #1722888 |
On Tue, Aug 29, 2017 at 3:15 PM, Dave Chinner <david@fromorbit.com> wrote: > If you are touching multiple filesystems, you really should cc the > entire patchset to linux-fsdevel, similar to how you sent the entire > patchset to lkml. That way the entire series will end up on a list > that almost all fs developers read. LKML is not a list you can rely > on all filesystem developers reading (or developers in any other > subsystem, for that matter)... Okay, sounds good. Thanks! -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | "Darrick J. Wong" <darrick.wong@oracle.com> |
|---|---|
| Date | 2017-08-29 00:00 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujAeB-2oT-13@gated-at.bofh.it> |
| In reply to | #1721998 |
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?
--D
> ...
>
> fs/xfs/xfs_symlink.c:
> xfs_symlink(...):
> ...
> xfs_init_local_fork(ip, XFS_DATA_FORK, target_path, pathlen);
>
> example usage trace:
> readlink_copy+0x43/0x70
> vfs_readlink+0x62/0x110
> SyS_readlinkat+0x100/0x130
>
> fs/xfs/xfs_iops.c:
> (via inode->i_op->get_link)
> xfs_vn_get_link_inline(...):
> ...
> return XFS_I(inode)->i_df.if_u1.if_data;
>
> fs/namei.c:
> readlink_copy(..., link):
> ...
> copy_to_user(..., link, len);
>
> generic_readlink(dentry, ...):
> struct inode *inode = d_inode(dentry);
> const char *link = inode->i_link;
> ...
> if (!link) {
> link = inode->i_op->get_link(dentry, inode, &done);
> ...
> readlink_copy(..., link);
>
> In support of usercopy hardening, this patch defines a region in the
> xfs_inode slab cache in which userspace copy operations are allowed.
>
> This region is known as the slab cache's usercopy region. Slab caches can
> now check that each copy operation involving cache-managed memory falls
> entirely within the slab's usercopy region.
>
> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
> whitelisting code in the last public patch of grsecurity/PaX based on my
> understanding of the code. Changes or omissions from the original code are
> mine and don't reflect the original grsecurity/PaX code.
>
> Signed-off-by: David Windsor <dave@nullcore.net>
> [kees: adjust commit log, provide usage trace]
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Cc: linux-xfs@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/xfs/kmem.h | 10 ++++++++++
> fs/xfs/xfs_super.c | 7 +++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h
> index 4d85992d75b2..08358f38dee6 100644
> --- a/fs/xfs/kmem.h
> +++ b/fs/xfs/kmem.h
> @@ -110,6 +110,16 @@ kmem_zone_init_flags(int size, char *zone_name, unsigned long flags,
> return kmem_cache_create(zone_name, size, 0, flags, construct);
> }
>
> +static inline kmem_zone_t *
> +kmem_zone_init_flags_usercopy(int size, char *zone_name, unsigned long flags,
> + size_t useroffset, size_t usersize,
> + void (*construct)(void *))
> +{
> + return kmem_cache_create_usercopy(zone_name, size, 0, flags,
> + useroffset, usersize, construct);
> +}
> +
> +
> static inline void
> kmem_zone_free(kmem_zone_t *zone, void *ptr)
> {
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 38aaacdbb8b3..6ca428c6f943 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1829,9 +1829,12 @@ xfs_init_zones(void)
> goto out_destroy_efd_zone;
>
> xfs_inode_zone =
> - kmem_zone_init_flags(sizeof(xfs_inode_t), "xfs_inode",
> + kmem_zone_init_flags_usercopy(sizeof(xfs_inode_t), "xfs_inode",
> KM_ZONE_HWALIGN | KM_ZONE_RECLAIM | KM_ZONE_SPREAD |
> - KM_ZONE_ACCOUNT, xfs_fs_inode_init_once);
> + KM_ZONE_ACCOUNT,
> + offsetof(xfs_inode_t, i_df.if_u2.if_inline_data),
> + sizeof_field(xfs_inode_t, i_df.if_u2.if_inline_data),
> + xfs_fs_inode_init_once);
> if (!xfs_inode_zone)
> goto out_destroy_efi_zone;
>
> --
> 2.7.4
>
> --
> 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
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-08-29 10:20 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujJUB-bY-13@gated-at.bofh.it> |
| In reply to | #1721998 |
One thing I've been wondering is wether we should actually just get rid of the online area. Compared to reading an inode from disk a single additional kmalloc is negligible, and not having the inline data / extent list would allow us to reduce the inode size significantly. Kees/David: how many of these patches are file systems with some sort of inline data? Given that it's only about 30 patches declaring allocations either entirely valid for user copy or not might end up being nicer in many ways than these offsets.
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2017-08-29 14:40 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujNYe-2CF-17@gated-at.bofh.it> |
| In reply to | #1722212 |
On Tue, Aug 29, 2017 at 01:14:53AM -0700, Christoph Hellwig wrote: > One thing I've been wondering is wether we should actually just > get rid of the online area. Compared to reading an inode from > disk a single additional kmalloc is negligible, and not having the > inline data / extent list would allow us to reduce the inode size > significantly. Probably should. I've already been looking at killing the inline extents array to simplify the management of the extent list (much simpler to index by rbtree when we don't have direct/indirect structures), so killing the inline data would get rid of the other part of the union the inline data sits in. OTOH, if we're going to have to dynamically allocate the memory for the extent/inline data for the data fork, it may just be easier to make the entire data fork a dynamic allocation (like the attr fork). Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-08-29 14:50 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujO7U-2G2-5@gated-at.bofh.it> |
| In reply to | #1722439 |
On Tue, Aug 29, 2017 at 10:31:26PM +1000, Dave Chinner wrote: > Probably should. I've already been looking at killing the inline > extents array to simplify the management of the extent list (much > simpler to index by rbtree when we don't have direct/indirect > structures), so killing the inline data would get rid of the other > part of the union the inline data sits in. That's exactly where I came form with my extent list work. Although the rbtree performance was horrible due to the memory overhead and I've switched to a modified b+tree at the moment.. > OTOH, if we're going to have to dynamically allocate the memory for > the extent/inline data for the data fork, it may just be easier to > make the entire data fork a dynamic allocation (like the attr fork). I though about this a bit, but it turned out that we basically always need the data anyway, so I don't think it's going to buy us much unless we shrink the inode enough so that they better fit into a page.
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2017-08-30 00:00 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujWIb-84B-45@gated-at.bofh.it> |
| In reply to | #1722441 |
On Tue, Aug 29, 2017 at 05:45:36AM -0700, Christoph Hellwig wrote: > On Tue, Aug 29, 2017 at 10:31:26PM +1000, Dave Chinner wrote: > > Probably should. I've already been looking at killing the inline > > extents array to simplify the management of the extent list (much > > simpler to index by rbtree when we don't have direct/indirect > > structures), so killing the inline data would get rid of the other > > part of the union the inline data sits in. > > That's exactly where I came form with my extent list work. Although > the rbtree performance was horrible due to the memory overhead and > I've switched to a modified b+tree at the moment.. Right, I've looked at btrees, too, but it's more complex than just using an rbtree. I originally looked at using Peter Z's old RCU-aware btree code, but it doesn't hold data in the tree leaves. So that needed significant modification to make work without a memory alloc per extent and that didn't work with original aim of RCU-safe extent lookups. I also looked at that "generic" btree stuff that came from logfs, and after a little while ran away screaming. So if we are going to use a b+tree, it sounds like you are probably going the right way. As it is, I've been looking at using interval tree - I have kinda working code - which basically leaves the page based extent arrays intact but adds an rbnode/interval state header to the start of each page to track the offsets within the node and propagate them back up to the root for fast offset based extent lookups. With a lookaside cache on the root, it should behave and perform almost identically to the current indirect array and should have very little extra overhead.... The sticking point, IMO, is the extent array index based lookups in all the bmbt code. I've been looking at converting all that to use offset based lookups and a cursor w/ lookup/inc/dec/insert/delete ioperations wrapping xfs_iext_lookup_ext() and friends. This means the modifications are pretty much identical to the on-disk extent btree, so they can be abstracted out into a single extent update interface for both trees. Have you planned/done any cleanup/changes with this code? > > OTOH, if we're going to have to dynamically allocate the memory for > > the extent/inline data for the data fork, it may just be easier to > > make the entire data fork a dynamic allocation (like the attr fork). > > I though about this a bit, but it turned out that we basically > always need the data anyway, so I don't think it's going to buy > us much unless we shrink the inode enough so that they better fit > into a page. True. Keep it mind for when we've shrunk the inode by another 100 bytes... Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-08-30 09:20 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <uk5s6-5jB-11@gated-at.bofh.it> |
| In reply to | #1722883 |
On Wed, Aug 30, 2017 at 07:51:57AM +1000, Dave Chinner wrote: > Right, I've looked at btrees, too, but it's more complex than just > using an rbtree. I originally looked at using Peter Z's old > RCU-aware btree code, but it doesn't hold data in the tree leaves. > So that needed significant modification to make work without a > memory alloc per extent and that didn't work with original aim of > RCU-safe extent lookups. I also looked at that "generic" btree > stuff that came from logfs, and after a little while ran away > screaming. I started with the latter, but it's not really looking like it any more: there nodes are formatted as a series of u64s instead of all the long magic, and the data is stored inline - in fact I use a cute trick to keep the size down, derived from our "compressed" on disk extent format: Key: +-------+----------------------------+ | 00:51 | all 52 bits of startoff | | 52:63 | low 12 bits of startblock | +-------+----------------------------+ Value +-------+----------------------------+ | 00:20 | all 21 bits of length | | 21 | unwritten extent bit | | 22:63 | high 42 bits of startblock | +-------+----------------------------+ So we only need a 64-bit key and a 64-bit value by abusing parts of the key to store bits of the startblock. For non-leaf nodes we iterate through the keys only, never touching the cache lines for the value. For the leaf nodes we have to touch the value anyway because we have to do a range lookup to find the exact record. This works fine so far in an isolated simulator, and now I'm ammending it to be a b+tree with pointers to the previous and next node so that we can nicely implement our extent iterators instead of doing full lookups. > The sticking point, IMO, is the extent array index based lookups in > all the bmbt code. I've been looking at converting all that to use > offset based lookups and a cursor w/ lookup/inc/dec/insert/delete > ioperations wrapping xfs_iext_lookup_ext() and friends. This means > the modifications are pretty much identical to the on-disk extent > btree, so they can be abstracted out into a single extent update > interface for both trees. Have you planned/done any cleanup/changes > with this code? I've done various cleanups, but I've not yet consolidated the two. Basically step one at the moment is to move everyone to xfs_iext_lookup_extent + xfs_iext_get_extent that removes all the bad intrusion. Once we move to the actual b+trees the extnum_t cursor will be replaced with a real cursor structure that contains a pointer to the current b+tree leaf node, and an index inside that, which will allows us very efficient iteration. The xfs_iext_get_extent calls will be replaced with more specific xfs_iext_prev_extent, xfs_iext_next_extent calls that include the now slightly more complex cursor decrement, increment as well as a new xfs_iext_last_extent helper for the last extent that we need in a few places. insert/delete remain very similar to what they do right now, they'll get a different cursor type, and the manual xfs_iext_add calls will go away. The new xfs_iext_update_extent helper I posted to the list yesterday will become a bit more complex, as changing the startoff will have to be propagated up the tree.
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2017-08-30 10:10 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <uk6et-5Ol-19@gated-at.bofh.it> |
| In reply to | #1723066 |
On Wed, Aug 30, 2017 at 12:14:03AM -0700, Christoph Hellwig wrote:
> On Wed, Aug 30, 2017 at 07:51:57AM +1000, Dave Chinner wrote:
> > Right, I've looked at btrees, too, but it's more complex than just
> > using an rbtree. I originally looked at using Peter Z's old
> > RCU-aware btree code, but it doesn't hold data in the tree leaves.
> > So that needed significant modification to make work without a
> > memory alloc per extent and that didn't work with original aim of
> > RCU-safe extent lookups. I also looked at that "generic" btree
> > stuff that came from logfs, and after a little while ran away
> > screaming.
>
> I started with the latter, but it's not really looking like it any more:
> there nodes are formatted as a series of u64s instead of all the
> long magic,
Yeah, that was about where I started to run away and look for
something nicer....
> and the data is stored inline - in fact I use a cute
> trick to keep the size down, derived from our "compressed" on disk
> extent format:
>
> Key:
>
> +-------+----------------------------+
> | 00:51 | all 52 bits of startoff |
> | 52:63 | low 12 bits of startblock |
> +-------+----------------------------+
>
> Value
>
> +-------+----------------------------+
> | 00:20 | all 21 bits of length |
> | 21 | unwritten extent bit |
> | 22:63 | high 42 bits of startblock |
> +-------+----------------------------+
>
> So we only need a 64-bit key and a 64-bit value by abusing parts
> of the key to store bits of the startblock.
Neat! :)
> For non-leaf nodes we iterate through the keys only, never touching
> the cache lines for the value. For the leaf nodes we have to touch
> the value anyway because we have to do a range lookup to find the
> exact record.
>
> This works fine so far in an isolated simulator, and now I'm ammending
> it to be a b+tree with pointers to the previous and next node so
> that we can nicely implement our extent iterators instead of doing
> full lookups.
Ok, that sounds exactly what I have been looking towards....
> > The sticking point, IMO, is the extent array index based lookups in
> > all the bmbt code. I've been looking at converting all that to use
> > offset based lookups and a cursor w/ lookup/inc/dec/insert/delete
> > ioperations wrapping xfs_iext_lookup_ext() and friends. This means
> > the modifications are pretty much identical to the on-disk extent
> > btree, so they can be abstracted out into a single extent update
> > interface for both trees. Have you planned/done any cleanup/changes
> > with this code?
>
> I've done various cleanups, but I've not yet consolidated the two.
> Basically step one at the moment is to move everyone to
> xfs_iext_lookup_extent + xfs_iext_get_extent that removes all the
> bad intrusion.
Yup.
> Once we move to the actual b+trees the extnum_t cursor will be replaced
> with a real cursor structure that contains a pointer to the current
> b+tree leaf node, and an index inside that, which will allows us very
> efficient iteration. The xfs_iext_get_extent calls will be replaced
> with more specific xfs_iext_prev_extent, xfs_iext_next_extent calls
> that include the now slightly more complex cursor decrement, increment
> as well as a new xfs_iext_last_extent helper for the last extent
> that we need in a few places.
Ok, that's sounds like it'll fit right in with what I've been
prototyping for the extent code in xfs_bmap.c. I can make that work
with a cursor-based lookup/inc/dec/ins/del API similar to the bmbt
API. I've been looking to abstract the extent manipulations out into
functions that modify both trees like this:
[note: just put template code in to get my thoughts straight, it's
not working code]
+static int
+xfs_bmex_delete(
+ struct xfs_iext_cursor *icur,
+ struct xfs_btree_cursor *cur,
+ int *nextents)
+{
+ int i;
+
+ xfs_iext_remove(bma->ip, bma->idx + 1, 2, state);
+ if (nextents)
+ (*nextents)--;
+ if (!cur)
+ return 0;
+ error = xfs_btree_delete(cur, &i);
+ if (error)
+ return error;
+ XFS_WANT_CORRUPTED_RETURN(cur->bc_mp, i == 1);
+ return 0;
+}
+
+static int
+xfs_bmex_increment(
+ struct xfs_iext_cursor *icur,
+ struct xfs_btree_cursor *cur)
+{
+ int i;
+
+ icur->ep = xfs_iext_get_right_ext(icur->ep);
+ if (!cur)
+ return 0;
+ error = xfs_btree_increment(cur, 0, &i);
+ if (error)
+ return error;
+ XFS_WANT_CORRUPTED_RETURN(cur->bc_mp, i == 1);
+ return 0;
+}
+
+static int
+xfs_bmex_decrement(
+ struct xfs_iext_cursor *icur,
+ struct xfs_btree_cursor *cur)
+{
+ int i;
+
+ icur->ep = xfs_iext_get_left_ext(icur->ep);
+ if (!cur)
+ return 0;
+ error = xfs_btree_decrement(cur, 0, &i);
+ if (error)
+ return error;
+ XFS_WANT_CORRUPTED_RETURN(cur->bc_mp, i == 1);
+ return 0;
+}
And so what you're doing would fit straight into that. I'm
ending up with is extent operations that look like this:
xfs_bmap_add_extent_delay_real()
.....
case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG |
BMAP_RIGHT_FILLING | BMAP_RIGHT_CONTIG:
/*
* Filling in all of a previously delayed allocation extent.
* The left and right neighbors are both contiguous with new.
*/
+ rval |= XFS_ILOG_CORE;
+
+ /* remove the incore delalloc extent first */
+ error = xfs_bmex_delete(&icur, NULL, nextents);
+ if (error)
+ goto done;
+
+ /*
+ * update incore and bmap extent trees
+ * 1. set cursors to the right extent
+ * 2. remove the right extent
+ * 3. update the left extent to span all 3 extent ranges
+ */
+ error = xfs_bmex_lookup_eq(&icur, bma->cur, RIGHT.br_startoff,
+ RIGHT.br_startblock, RIGHT.br_blockcount, 1);
+ if (error)
+ goto done;
+ error = xfs_bmex_delete(&icur, bma->cur, NULL);
+ if (error)
+ goto done;
+ error = xfs_bmex_decrement(&icur, bma->cur);
+ if (error)
+ goto done;
+ error = xfs_bmex_update(&icur, bma->cur, LEFT.br_startoff,
+ LEFT.br_startblock,
+ LEFT.br_blockcount + PREV.br_blockcount +
+ RIGHT.br_blockcount,
+ LEFT.br_state);
+ if (error)
+ goto done;
break;
....
And I'm starting to see where there are common extent manipulations
being done so there's probably a fair amount of further factoring
that can be done on top of this....
> insert/delete remain very similar to what they do right now, they'll
> get a different cursor type, and the manual xfs_iext_add calls will
> go away. The new xfs_iext_update_extent helper I posted to the list
> yesterday will become a bit more complex, as changing the startoff
> will have to be propagated up the tree.
I've had a quick look at them and pulled it down into my tree for
testing (which had a cpu burning hang on xfs/020 a few minutes ago),
but I'll spend more time grokking them tomorrow.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2017-08-30 10:40 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <uk6Hw-5Y8-19@gated-at.bofh.it> |
| In reply to | #1723086 |
On Wed, Aug 30, 2017 at 06:05:58PM +1000, Dave Chinner wrote:
> Ok, that's sounds like it'll fit right in with what I've been
> prototyping for the extent code in xfs_bmap.c. I can make that work
> with a cursor-based lookup/inc/dec/ins/del API similar to the bmbt
> API. I've been looking to abstract the extent manipulations out into
> functions that modify both trees like this:
>
> [note: just put template code in to get my thoughts straight, it's
> not working code]
FYI, I've got somewhat working changes in that area (still has bugs
but a few tests pass :)), what I'm doing is to make sure all of
the xfs_bmap_{add,del}_extent_* routines fully operate on xfs_bmbt_irec
structures that they acquire through the xfs_bmalloca structure or
from xfs_iext_get_extent and update using xfs_iext_update_extent.
A nice fallout from that is that we can change the prototypes for
xfs_bmbt_lookup_* and xfs_bmbt_update to take a xfs_bmbt_irec
as well instead of taking the individual arguments. That should
help with your next step cleanups a bit.
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-29 21:00 +0200 |
| Subject | Re: [PATCH v2 15/30] xfs: Define usercopy region in xfs_inode slab cache |
| Message-ID | <ujTTY-6ib-21@gated-at.bofh.it> |
| In reply to | #1722212 |
On Tue, Aug 29, 2017 at 1:14 AM, Christoph Hellwig <hch@infradead.org> wrote: > One thing I've been wondering is wether we should actually just > get rid of the online area. Compared to reading an inode from > disk a single additional kmalloc is negligible, and not having the > inline data / extent list would allow us to reduce the inode size > significantly. > > Kees/David: how many of these patches are file systems with some > sort of inline data? Given that it's only about 30 patches declaring > allocations either entirely valid for user copy or not might end up > being nicer in many ways than these offsets. 9 filesystems use some form of inline data: xfs, vxfs, ufs, orangefs, exofs, befs, jfs, ext2, and ext4. How much of each slab is whitelisted varies by filesystem (e.g. ext2/4 uses i_data for other things, but ufs and orangefs and have a dedicate field for symlink names). -Kees -- Kees Cook Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:40 +0200 |
| Subject | [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache slab cache |
| Message-ID | <ujzVg-2ie-43@gated-at.bofh.it> |
| In reply to | #1721983 |
From: David Windsor <dave@nullcore.net>
befs symlink pathnames, stored in struct befs_inode_info.i_data.symlink
and therefore contained in the befs_inode_cache slab cache, need to be
copied to/from userspace.
cache object allocation:
fs/befs/linuxvfs.c:
befs_alloc_inode(...):
...
bi = kmem_cache_alloc(befs_inode_cachep, GFP_KERNEL);
...
return &bi->vfs_inode;
befs_iget(...):
...
strlcpy(befs_ino->i_data.symlink, raw_inode->data.symlink,
BEFS_SYMLINK_LEN);
...
inode->i_link = befs_ino->i_data.symlink;
example usage trace:
readlink_copy+0x43/0x70
vfs_readlink+0x62/0x110
SyS_readlinkat+0x100/0x130
fs/namei.c:
readlink_copy(..., link):
...
copy_to_user(..., link, len);
(inlined in vfs_readlink)
generic_readlink(dentry, ...):
struct inode *inode = d_inode(dentry);
const char *link = inode->i_link;
...
readlink_copy(..., link);
In support of usercopy hardening, this patch defines a region in the
befs_inode_cache slab cache in which userspace copy operations are
allowed.
This region is known as the slab cache's usercopy region. Slab caches can
now check that each copy operation involving cache-managed memory falls
entirely within the slab's usercopy region.
This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
whitelisting code in the last public patch of grsecurity/PaX based on my
understanding of the code. Changes or omissions from the original code are
mine and don't reflect the original grsecurity/PaX code.
Signed-off-by: David Windsor <dave@nullcore.net>
[kees: adjust commit log, provide usage trace]
Cc: Luis de Bethencourt <luisbg@kernel.org>
Cc: Salah Triki <salah.triki@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/befs/linuxvfs.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index 4a4a5a366158..1c2dcbee79dd 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -444,11 +444,15 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino)
static int __init
befs_init_inodecache(void)
{
- befs_inode_cachep = kmem_cache_create("befs_inode_cache",
- sizeof (struct befs_inode_info),
- 0, (SLAB_RECLAIM_ACCOUNT|
- SLAB_MEM_SPREAD|SLAB_ACCOUNT),
- init_once);
+ befs_inode_cachep = kmem_cache_create_usercopy("befs_inode_cache",
+ sizeof(struct befs_inode_info), 0,
+ (SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD|
+ SLAB_ACCOUNT),
+ offsetof(struct befs_inode_info,
+ i_data.symlink),
+ sizeof_field(struct befs_inode_info,
+ i_data.symlink),
+ init_once);
if (befs_inode_cachep == NULL)
return -ENOMEM;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Luis de Bethencourt <luisbg@kernel.org> |
|---|---|
| Date | 2017-08-29 12:20 +0200 |
| Subject | Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache slab cache |
| Message-ID | <ujLMK-1lA-9@gated-at.bofh.it> |
| In reply to | #1721999 |
Hello Kees,
This is great. Thanks :)
Will merge into my befs tree.
Luis
On 08/28/2017 10:34 PM, Kees Cook wrote:
> From: David Windsor <dave@nullcore.net>
>
> befs symlink pathnames, stored in struct befs_inode_info.i_data.symlink
> and therefore contained in the befs_inode_cache slab cache, need to be
> copied to/from userspace.
>
> cache object allocation:
> fs/befs/linuxvfs.c:
> befs_alloc_inode(...):
> ...
> bi = kmem_cache_alloc(befs_inode_cachep, GFP_KERNEL);
> ...
> return &bi->vfs_inode;
>
> befs_iget(...):
> ...
> strlcpy(befs_ino->i_data.symlink, raw_inode->data.symlink,
> BEFS_SYMLINK_LEN);
> ...
> inode->i_link = befs_ino->i_data.symlink;
>
> example usage trace:
> readlink_copy+0x43/0x70
> vfs_readlink+0x62/0x110
> SyS_readlinkat+0x100/0x130
>
> fs/namei.c:
> readlink_copy(..., link):
> ...
> copy_to_user(..., link, len);
>
> (inlined in vfs_readlink)
> generic_readlink(dentry, ...):
> struct inode *inode = d_inode(dentry);
> const char *link = inode->i_link;
> ...
> readlink_copy(..., link);
>
> In support of usercopy hardening, this patch defines a region in the
> befs_inode_cache slab cache in which userspace copy operations are
> allowed.
>
> This region is known as the slab cache's usercopy region. Slab caches can
> now check that each copy operation involving cache-managed memory falls
> entirely within the slab's usercopy region.
>
> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
> whitelisting code in the last public patch of grsecurity/PaX based on my
> understanding of the code. Changes or omissions from the original code are
> mine and don't reflect the original grsecurity/PaX code.
>
> Signed-off-by: David Windsor <dave@nullcore.net>
> [kees: adjust commit log, provide usage trace]
> Cc: Luis de Bethencourt <luisbg@kernel.org>
> Cc: Salah Triki <salah.triki@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/befs/linuxvfs.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
> index 4a4a5a366158..1c2dcbee79dd 100644
> --- a/fs/befs/linuxvfs.c
> +++ b/fs/befs/linuxvfs.c
> @@ -444,11 +444,15 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino)
> static int __init
> befs_init_inodecache(void)
> {
> - befs_inode_cachep = kmem_cache_create("befs_inode_cache",
> - sizeof (struct befs_inode_info),
> - 0, (SLAB_RECLAIM_ACCOUNT|
> - SLAB_MEM_SPREAD|SLAB_ACCOUNT),
> - init_once);
> + befs_inode_cachep = kmem_cache_create_usercopy("befs_inode_cache",
> + sizeof(struct befs_inode_info), 0,
> + (SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD|
> + SLAB_ACCOUNT),
> + offsetof(struct befs_inode_info,
> + i_data.symlink),
> + sizeof_field(struct befs_inode_info,
> + i_data.symlink),
> + init_once);
> if (befs_inode_cachep == NULL)
> return -ENOMEM;
>
>
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-29 17:40 +0200 |
| Subject | Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache slab cache |
| Message-ID | <ujQMp-4mN-1@gated-at.bofh.it> |
| In reply to | #1722300 |
On Tue, Aug 29, 2017 at 3:12 AM, Luis de Bethencourt <luisbg@kernel.org> wrote:
> Hello Kees,
>
> This is great. Thanks :)
>
> Will merge into my befs tree.
Hi! Actually, this depends on the rest of the series, which should be
merged together. If you can Ack this, I'll include it in my usercopy
tree.
Thanks!
-Kees
>
> Luis
>
>
> On 08/28/2017 10:34 PM, Kees Cook wrote:
>>
>> From: David Windsor <dave@nullcore.net>
>>
>> befs symlink pathnames, stored in struct befs_inode_info.i_data.symlink
>> and therefore contained in the befs_inode_cache slab cache, need to be
>> copied to/from userspace.
>>
>> cache object allocation:
>> fs/befs/linuxvfs.c:
>> befs_alloc_inode(...):
>> ...
>> bi = kmem_cache_alloc(befs_inode_cachep, GFP_KERNEL);
>> ...
>> return &bi->vfs_inode;
>>
>> befs_iget(...):
>> ...
>> strlcpy(befs_ino->i_data.symlink, raw_inode->data.symlink,
>> BEFS_SYMLINK_LEN);
>> ...
>> inode->i_link = befs_ino->i_data.symlink;
>>
>> example usage trace:
>> readlink_copy+0x43/0x70
>> vfs_readlink+0x62/0x110
>> SyS_readlinkat+0x100/0x130
>>
>> fs/namei.c:
>> readlink_copy(..., link):
>> ...
>> copy_to_user(..., link, len);
>>
>> (inlined in vfs_readlink)
>> generic_readlink(dentry, ...):
>> struct inode *inode = d_inode(dentry);
>> const char *link = inode->i_link;
>> ...
>> readlink_copy(..., link);
>>
>> In support of usercopy hardening, this patch defines a region in the
>> befs_inode_cache slab cache in which userspace copy operations are
>> allowed.
>>
>> This region is known as the slab cache's usercopy region. Slab caches can
>> now check that each copy operation involving cache-managed memory falls
>> entirely within the slab's usercopy region.
>>
>> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
>> whitelisting code in the last public patch of grsecurity/PaX based on my
>> understanding of the code. Changes or omissions from the original code are
>> mine and don't reflect the original grsecurity/PaX code.
>>
>> Signed-off-by: David Windsor <dave@nullcore.net>
>> [kees: adjust commit log, provide usage trace]
>> Cc: Luis de Bethencourt <luisbg@kernel.org>
>> Cc: Salah Triki <salah.triki@gmail.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> fs/befs/linuxvfs.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
>> index 4a4a5a366158..1c2dcbee79dd 100644
>> --- a/fs/befs/linuxvfs.c
>> +++ b/fs/befs/linuxvfs.c
>> @@ -444,11 +444,15 @@ static struct inode *befs_iget(struct super_block
>> *sb, unsigned long ino)
>> static int __init
>> befs_init_inodecache(void)
>> {
>> - befs_inode_cachep = kmem_cache_create("befs_inode_cache",
>> - sizeof (struct
>> befs_inode_info),
>> - 0, (SLAB_RECLAIM_ACCOUNT|
>> -
>> SLAB_MEM_SPREAD|SLAB_ACCOUNT),
>> - init_once);
>> + befs_inode_cachep = kmem_cache_create_usercopy("befs_inode_cache",
>> + sizeof(struct befs_inode_info), 0,
>> + (SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD|
>> + SLAB_ACCOUNT),
>> + offsetof(struct befs_inode_info,
>> + i_data.symlink),
>> + sizeof_field(struct befs_inode_info,
>> + i_data.symlink),
>> + init_once);
>> if (befs_inode_cachep == NULL)
>> return -ENOMEM;
>>
>
>
--
Kees Cook
Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Luis de Bethencourt <luisbg@kernel.org> |
|---|---|
| Date | 2017-08-29 19:20 +0200 |
| Subject | Re: [PATCH v2 10/30] befs: Define usercopy region in befs_inode_cache slab cache |
| Message-ID | <ujSlc-5rP-21@gated-at.bofh.it> |
| In reply to | #1722563 |
On 08/29/2017 04:36 PM, Kees Cook wrote:
> On Tue, Aug 29, 2017 at 3:12 AM, Luis de Bethencourt <luisbg@kernel.org> wrote:
>> Hello Kees,
>>
>> This is great. Thanks :)
>>
>> Will merge into my befs tree.
>
> Hi! Actually, this depends on the rest of the series, which should be
> merged together. If you can Ack this, I'll include it in my usercopy
> tree.
>
> Thanks!
>
> -Kees
>
Sure!
Acked-by: Luis de Bethencourt <luisbg@kernel.org>
>>
>> Luis
>>
>>
>> On 08/28/2017 10:34 PM, Kees Cook wrote:
>>>
>>> From: David Windsor <dave@nullcore.net>
>>>
>>> befs symlink pathnames, stored in struct befs_inode_info.i_data.symlink
>>> and therefore contained in the befs_inode_cache slab cache, need to be
>>> copied to/from userspace.
>>>
>>> cache object allocation:
>>> fs/befs/linuxvfs.c:
>>> befs_alloc_inode(...):
>>> ...
>>> bi = kmem_cache_alloc(befs_inode_cachep, GFP_KERNEL);
>>> ...
>>> return &bi->vfs_inode;
>>>
>>> befs_iget(...):
>>> ...
>>> strlcpy(befs_ino->i_data.symlink, raw_inode->data.symlink,
>>> BEFS_SYMLINK_LEN);
>>> ...
>>> inode->i_link = befs_ino->i_data.symlink;
>>>
>>> example usage trace:
>>> readlink_copy+0x43/0x70
>>> vfs_readlink+0x62/0x110
>>> SyS_readlinkat+0x100/0x130
>>>
>>> fs/namei.c:
>>> readlink_copy(..., link):
>>> ...
>>> copy_to_user(..., link, len);
>>>
>>> (inlined in vfs_readlink)
>>> generic_readlink(dentry, ...):
>>> struct inode *inode = d_inode(dentry);
>>> const char *link = inode->i_link;
>>> ...
>>> readlink_copy(..., link);
>>>
>>> In support of usercopy hardening, this patch defines a region in the
>>> befs_inode_cache slab cache in which userspace copy operations are
>>> allowed.
>>>
>>> This region is known as the slab cache's usercopy region. Slab caches can
>>> now check that each copy operation involving cache-managed memory falls
>>> entirely within the slab's usercopy region.
>>>
>>> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
>>> whitelisting code in the last public patch of grsecurity/PaX based on my
>>> understanding of the code. Changes or omissions from the original code are
>>> mine and don't reflect the original grsecurity/PaX code.
>>>
>>> Signed-off-by: David Windsor <dave@nullcore.net>
>>> [kees: adjust commit log, provide usage trace]
>>> Cc: Luis de Bethencourt <luisbg@kernel.org>
>>> Cc: Salah Triki <salah.triki@gmail.com>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> fs/befs/linuxvfs.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
>>> index 4a4a5a366158..1c2dcbee79dd 100644
>>> --- a/fs/befs/linuxvfs.c
>>> +++ b/fs/befs/linuxvfs.c
>>> @@ -444,11 +444,15 @@ static struct inode *befs_iget(struct super_block
>>> *sb, unsigned long ino)
>>> static int __init
>>> befs_init_inodecache(void)
>>> {
>>> - befs_inode_cachep = kmem_cache_create("befs_inode_cache",
>>> - sizeof (struct
>>> befs_inode_info),
>>> - 0, (SLAB_RECLAIM_ACCOUNT|
>>> -
>>> SLAB_MEM_SPREAD|SLAB_ACCOUNT),
>>> - init_once);
>>> + befs_inode_cachep = kmem_cache_create_usercopy("befs_inode_cache",
>>> + sizeof(struct befs_inode_info), 0,
>>> + (SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD|
>>> + SLAB_ACCOUNT),
>>> + offsetof(struct befs_inode_info,
>>> + i_data.symlink),
>>> + sizeof_field(struct befs_inode_info,
>>> + i_data.symlink),
>>> + init_once);
>>> if (befs_inode_cachep == NULL)
>>> return -ENOMEM;
>>>
>>
>>
>
>
>
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:50 +0200 |
| Subject | [PATCH v2 25/30] fork: Define usercopy region in thread_stack slab caches |
| Message-ID | <ujA4V-2lA-1@gated-at.bofh.it> |
| In reply to | #1721983 |
From: David Windsor <dave@nullcore.net>
In support of usercopy hardening, this patch defines a region in the
thread_stack slab caches in which userspace copy operations are allowed.
Since the entire thread_stack needs to be available to userspace, the
entire slab contents are whitelisted. Note that the slab-based thread
stack is only present on systems with THREAD_SIZE < PAGE_SIZE and
!CONFIG_VMAP_STACK.
cache object allocation:
kernel/fork.c:
alloc_thread_stack_node(...):
return kmem_cache_alloc_node(thread_stack_cache, ...)
dup_task_struct(...):
...
stack = alloc_thread_stack_node(...)
...
tsk->stack = stack;
copy_process(...):
...
dup_task_struct(...)
_do_fork(...):
...
copy_process(...)
This region is known as the slab cache's usercopy region. Slab caches
can now check that each copy operation involving cache-managed memory
falls entirely within the slab's usercopy region.
This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
whitelisting code in the last public patch of grsecurity/PaX based on my
understanding of the code. Changes or omissions from the original code are
mine and don't reflect the original grsecurity/PaX code.
Signed-off-by: David Windsor <dave@nullcore.net>
[kees: adjust commit log, split patch, provide usage trace]
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/fork.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index d8ebf755a47b..0f33fb1aabbf 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -276,8 +276,9 @@ static void free_thread_stack(struct task_struct *tsk)
void thread_stack_cache_init(void)
{
- thread_stack_cache = kmem_cache_create("thread_stack", THREAD_SIZE,
- THREAD_SIZE, 0, NULL);
+ thread_stack_cache = kmem_cache_create_usercopy("thread_stack",
+ THREAD_SIZE, THREAD_SIZE, 0, 0,
+ THREAD_SIZE, NULL);
BUG_ON(thread_stack_cache == NULL);
}
# endif
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Rik van Riel <riel@redhat.com> |
|---|---|
| Date | 2017-08-30 21:00 +0200 |
| Subject | Re: [kernel-hardening] [PATCH v2 25/30] fork: Define usercopy region in thread_stack slab caches |
| Message-ID | <ukgnw-3xh-47@gated-at.bofh.it> |
| In reply to | #1722000 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, 2017-08-28 at 14:35 -0700, Kees Cook wrote: > From: David Windsor <dave@nullcore.net> > > In support of usercopy hardening, this patch defines a region in the > thread_stack slab caches in which userspace copy operations are > allowed. > Since the entire thread_stack needs to be available to userspace, the > entire slab contents are whitelisted. Note that the slab-based thread > stack is only present on systems with THREAD_SIZE < PAGE_SIZE and > !CONFIG_VMAP_STACK. > Acked-by: Rik van Riel <riel@redhat.com> -- All rights reversed
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:50 +0200 |
| Subject | [PATCH v2 21/30] sctp: Define usercopy region in SCTP proto slab cache |
| Message-ID | <ujA4V-2lA-3@gated-at.bofh.it> |
| In reply to | #1721983 |
From: David Windsor <dave@nullcore.net>
The SCTP socket event notification subscription information need to be
copied to/from userspace. In support of usercopy hardening, this patch
defines a region in the struct proto slab cache in which userspace copy
operations are allowed. Additionally moves the usercopy fields to be
adjacent for the region to cover both.
example usage trace:
net/sctp/socket.c:
sctp_getsockopt_events(...):
...
copy_to_user(..., &sctp_sk(sk)->subscribe, len)
sctp_setsockopt_events(...):
...
copy_from_user(&sctp_sk(sk)->subscribe, ..., optlen)
sctp_getsockopt_initmsg(...):
...
copy_to_user(..., &sctp_sk(sk)->initmsg, len)
This region is known as the slab cache's usercopy region. Slab caches can
now check that each copy operation involving cache-managed memory falls
entirely within the slab's usercopy region.
This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
whitelisting code in the last public patch of grsecurity/PaX based on my
understanding of the code. Changes or omissions from the original code are
mine and don't reflect the original grsecurity/PaX code.
Signed-off-by: David Windsor <dave@nullcore.net>
[kees: split from network patch, move struct member adjacent, provide usage]
Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-sctp@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/net/sctp/structs.h | 9 +++++++--
net/sctp/socket.c | 4 ++++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 5ab29af8ca8a..f1d7810e200e 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -202,12 +202,17 @@ struct sctp_sock {
/* Flags controlling Heartbeat, SACK delay, and Path MTU Discovery. */
__u32 param_flags;
- struct sctp_initmsg initmsg;
struct sctp_rtoinfo rtoinfo;
struct sctp_paddrparams paddrparam;
- struct sctp_event_subscribe subscribe;
struct sctp_assocparams assocparams;
+ /*
+ * These two structures must be grouped together for the usercopy
+ * whitelist region.
+ */
+ struct sctp_event_subscribe subscribe;
+ struct sctp_initmsg initmsg;
+
int user_frag;
__u32 autoclose;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 1db478e34520..c8784cb216e4 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -8235,6 +8235,10 @@ struct proto sctp_prot = {
.unhash = sctp_unhash,
.get_port = sctp_get_port,
.obj_size = sizeof(struct sctp_sock),
+ .useroffset = offsetof(struct sctp_sock, subscribe),
+ .usersize = offsetof(struct sctp_sock, initmsg) -
+ offsetof(struct sctp_sock, subscribe) +
+ sizeof_field(struct sctp_sock, initmsg),
.sysctl_mem = sysctl_sctp_mem,
.sysctl_rmem = sysctl_sctp_rmem,
.sysctl_wmem = sysctl_sctp_wmem,
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:50 +0200 |
| Subject | [PATCH v2 19/30] ip: Define usercopy region in IP proto slab cache |
| Message-ID | <ujA4V-2lA-13@gated-at.bofh.it> |
| In reply to | #1721983 |
From: David Windsor <dave@nullcore.net>
The ICMP filters for IPv4 and IPv6 raw sockets need to be copied to/from
userspace. In support of usercopy hardening, this patch defines a region
in the struct proto slab cache in which userspace copy operations are
allowed.
example usage trace:
net/ipv4/raw.c:
raw_seticmpfilter(...):
...
copy_from_user(&raw_sk(sk)->filter, ..., optlen)
raw_geticmpfilter(...):
...
copy_to_user(..., &raw_sk(sk)->filter, len)
net/ipv6/raw.c:
rawv6_seticmpfilter(...):
...
copy_from_user(&raw6_sk(sk)->filter, ..., optlen)
rawv6_geticmpfilter(...):
...
copy_to_user(..., &raw6_sk(sk)->filter, len)
This region is known as the slab cache's usercopy region. Slab caches can
now check that each copy operation involving cache-managed memory falls
entirely within the slab's usercopy region.
This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY
whitelisting code in the last public patch of grsecurity/PaX based on my
understanding of the code. Changes or omissions from the original code are
mine and don't reflect the original grsecurity/PaX code.
Signed-off-by: David Windsor <dave@nullcore.net>
[kees: split from network patch, provide usage trace]
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
net/ipv4/raw.c | 2 ++
net/ipv6/raw.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index b0bb5d0a30bd..6c7f8d2eb3af 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -964,6 +964,8 @@ struct proto raw_prot = {
.hash = raw_hash_sk,
.unhash = raw_unhash_sk,
.obj_size = sizeof(struct raw_sock),
+ .useroffset = offsetof(struct raw_sock, filter),
+ .usersize = sizeof_field(struct raw_sock, filter),
.h.raw_hash = &raw_v4_hashinfo,
#ifdef CONFIG_COMPAT
.compat_setsockopt = compat_raw_setsockopt,
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 60be012fe708..27dd9a5f71c6 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -1265,6 +1265,8 @@ struct proto rawv6_prot = {
.hash = raw_hash_sk,
.unhash = raw_unhash_sk,
.obj_size = sizeof(struct raw6_sock),
+ .useroffset = offsetof(struct raw6_sock, filter),
+ .usersize = sizeof_field(struct raw6_sock, filter),
.h.raw_hash = &raw_v6_hashinfo,
#ifdef CONFIG_COMPAT
.compat_setsockopt = compat_rawv6_setsockopt,
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:50 +0200 |
| Subject | [PATCH v2 22/30] sctp: Copy struct sctp_sock.autoclose to userspace using put_user() |
| Message-ID | <ujA4V-2lA-9@gated-at.bofh.it> |
| In reply to | #1721983 |
From: David Windsor <dave@nullcore.net> The autoclose field can be copied with put_user(), so there is no need to use copy_to_user(). In both cases, hardened usercopy is being bypassed since the size is constant, and not open to runtime manipulation. This patch is verbatim from Brad Spengler/PaX Team's PAX_USERCOPY whitelisting code in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code. Signed-off-by: David Windsor <dave@nullcore.net> [kees: adjust commit log] Cc: Vlad Yasevich <vyasevich@gmail.com> Cc: Neil Horman <nhorman@tuxdriver.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: linux-sctp@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- net/sctp/socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index c8784cb216e4..a29e41e19d64 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4882,7 +4882,7 @@ static int sctp_getsockopt_autoclose(struct sock *sk, int len, char __user *optv len = sizeof(int); if (put_user(len, optlen)) return -EFAULT; - if (copy_to_user(optval, &sctp_sk(sk)->autoclose, sizeof(int))) + if (put_user(sctp_sk(sk)->autoclose, (int __user *)optval)) return -EFAULT; return 0; } -- 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-28 23:50 +0200 |
| Subject | [PATCH v2 29/30] arm: Implement thread_struct whitelist for hardened usercopy |
| Message-ID | <ujA4V-2lA-15@gated-at.bofh.it> |
| In reply to | #1721983 |
ARM does not carry FPU state in the thread structure, so it can declare
no usercopy whitelist at all.
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/processor.h | 7 +++++++
2 files changed, 8 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a208bfe367b5..3781f08d00fa 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -48,6 +48,7 @@ config ARM
select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
select HAVE_ARCH_MMAP_RND_BITS if MMU
select HAVE_ARCH_SECCOMP_FILTER if (AEABI && !OABI_COMPAT)
+ select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_TRACEHOOK
select HAVE_ARM_SMCCC if CPU_V7
select HAVE_CBPF_JIT
diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index c3d5fc124a05..d6dc45c92ee5 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -45,6 +45,13 @@ struct thread_struct {
struct debug_info debug;
};
+/* Nothing needs to be usercopy-whitelisted from thread_struct. */
+static inline void arch_thread_struct_whitelist(unsigned long *offset,
+ unsigned long *size)
+{
+ *offset = *size = 0;
+}
+
#define INIT_THREAD { }
#ifdef CONFIG_MMU
--
2.7.4
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web