* Re: [PATCH v2 0/3] initramfs: add support for xattrs in the initial ram disk
From: hpa @ 2019-05-13 0:23 UTC (permalink / raw)
To: Dominik Brodowski, Mimi Zohar
Cc: Roberto Sassu, viro, linux-security-module, linux-integrity,
initramfs, linux-api, linux-fsdevel, linux-kernel, zohar,
silviu.vlasceanu, dmitry.kasatkin, takondra, kamensky, arnd, rob,
james.w.mcmechan
In-Reply-To: <20190512153105.GA25254@light.dominikbrodowski.net>
On May 12, 2019 8:31:05 AM PDT, Dominik Brodowski <linux@dominikbrodowski.net> wrote:
>On Sun, May 12, 2019 at 03:18:16AM -0700, hpa@zytor.com wrote:
>> > Couldn't this parsing of the .xattr-list file and the setting of
>the xattrs
>> > be done equivalently by the initramfs' /init? Why is kernel
>involvement
>> > actually required here?
>>
>> There are a lot of things that could/should be done that way...
>
>Indeed... so why not try to avoid adding more such "things", and
>keeping
>them in userspace (or in a fork_usermode_blob)?
>
>
>On Sun, May 12, 2019 at 08:52:47AM -0400, Mimi Zohar wrote:
>> It's too late. The /init itself should be signed and verified.
>
>Could you elaborate a bit more about the threat model, and why
>deferring
>this to the initramfs is too late?
>
>Thanks,
> Dominik
I tried over 10 years ago to make exactly that happen... it was called the klibc project. Linus turned it down because he felt that it didn't provide enough immediate benefit to justify the complexity, which of course creates the thousand-cuts problem: there will never be *one single* event that *by itself* justifies the transition.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro
From: Ardelean, Alexandru @ 2019-05-13 7:00 UTC (permalink / raw)
To: andriy.shevchenko@linux.intel.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-ide@vger.kernel.org, linux-usb@vger.kernel.org,
linux-mmc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
cgroups@vger.kernel.org, intel-gfx@lists.freedesktop.org,
linux-pm@vger.kernel.org, linux-mm@kvack.org,
linux-omap@vger.kernel.org, linux-gpio@vger.kernel.org,
linux-security-module@vger.kernel.org, devel@driverdev.osuosl.org,
linux-mtd@lists.infradead.org, linux-integrity@vger.kernel.org,
dri-devel@lists.freedesktop.org,
linux-rpi-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
gregkh@linuxfoundation.org, linux-arm-kernel@lists.infradead.org,
linux-tegra@vger.kernel.org, netdev@vger.kernel.org,
linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org,
linux-pci@vger.kernel.org, linux-wireless@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <20190510143407.GA9224@smile.fi.intel.com>
On Fri, 2019-05-10 at 17:34 +0300, andriy.shevchenko@linux.intel.com wrote:
> [External]
>
>
> On Fri, May 10, 2019 at 09:15:27AM +0000, Ardelean, Alexandru wrote:
> > On Wed, 2019-05-08 at 16:22 +0300, Alexandru Ardelean wrote:
> > > On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote:
> > > > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean
> > > > > wrote:
> > > > > Can you split include/linux/ change from the rest?
> > > >
> > > > That would break the build, why do you want it split out? This
> > > > makes
> > > > sense all as a single patch to me.
> > > >
> > >
> > > Not really.
> > > It would be just be the new match_string() helper/macro in a new
> > > commit.
> > > And the conversions of the simple users of match_string() (the ones
> > > using
> > > ARRAY_SIZE()) in another commit.
> > >
> >
> > I should have asked in my previous reply.
> > Leave this as-is or re-formulate in 2 patches ?
>
> Depends on on what you would like to spend your time: collecting Acks for
> all
> pieces in treewide patch or send new API first followed up by per driver
> /
> module update in next cycle.
I actually would have preferred new API first, with the current
`match_string()` -> `__match_string()` rename from the start, but I wasn't
sure. I am still navigating through how feedbacks are working in this
realm.
I'll send a V2 with the API change-first/only; should be a smaller list.
Then see about follow-ups/changes per subsystems.
>
> I also have no strong preference.
> And I think it's good to add Heikki Krogerus to Cc list for both patch
> series,
> since he is the author of sysfs variant and may have something to comment
> on
> the rest.
Thanks for the reference.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* [PATCH 0/5] integrity: improve ima measurement accuracy
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar; +Cc: Janne Karhunen
By default the linux integrity subsystem measures a file only
when a file is being closed. While this certainly provides
low overhead as the re-measurements are never done, it also
makes sure the system has zero means to recover from a crash
or a power outage when operating in 'appraise' mode.
This patch series adds two new IMA api functions to retrigger
the measurements as the files change. Synchronous variant
should be invoked from less performance sensitive locations
such as sync|msync|truncate where the user is expecting some
latency, and the asynchronous variant can be called from
performance sensitive locations such as direct write or mmio.
Asynchronous variant is mostly 'out of the way' on write hot
paths, each file write is only checking that we have a cmwq
work entry pending to re-calculate the file measurement later
on. Re-measurement latencies are build time tunables and the
latencies are automatically raised for very large files.
While this does not provide absolutely perfect tolerance to
system resets, for most reasonable embedded system workloads
it can be tuned to achieve really high measurement accurancy
with the measurements being accurate 99.9%+ of the day.
Janne Karhunen (5):
integrity: keep the integrity state of open files up to date
integrity: update the file measurement on truncate
integrity: update the file measurement on write
integrity: measure the file on sync
integrity: measure the file on msync
fs/namei.c | 5 +-
fs/open.c | 3 +
fs/read_write.c | 11 ++-
fs/sync.c | 3 +
include/linux/ima.h | 12 +++
mm/msync.c | 7 ++
security/integrity/ima/Kconfig | 20 +++++
security/integrity/ima/ima_appraise.c | 6 +-
security/integrity/ima/ima_main.c | 103 +++++++++++++++++++++++++-
security/integrity/integrity.h | 6 ++
10 files changed, 171 insertions(+), 5 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 2/5] integrity: update the file measurement on truncate
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190513125354.23126-1-janne.karhunen@gmail.com>
Let IMA know when a file is being opened with truncate
or truncated directly.
Depends on commit c8213962517e ("integrity: keep the integrity state of open files up to date")'
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
fs/namei.c | 5 ++++-
fs/open.c | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index dede0147b3f6..31303063143b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3418,8 +3418,11 @@ static int do_last(struct nameidata *nd,
goto out;
opened:
error = ima_file_check(file, op->acc_mode);
- if (!error && will_truncate)
+ if (!error && will_truncate) {
error = handle_truncate(file);
+ if (!error)
+ ima_file_update(file);
+ }
out:
if (unlikely(error > 0)) {
WARN_ON(1);
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..a2771b787383 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -62,6 +62,9 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
/* Note any delegations or leases have already been broken: */
ret = notify_change(dentry, &newattrs, NULL);
inode_unlock(dentry->d_inode);
+
+ if (filp)
+ ima_file_update(filp);
return ret;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/5] integrity: keep the integrity state of open files up to date
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190513125354.23126-1-janne.karhunen@gmail.com>
When a file is open for writing, kernel crash or power outage
is guaranteed to corrupt the inode integrity state leading to
file appraisal failure on the subsequent boot. Add some basic
infrastructure to keep the integrity measurements up to date
as the files are written to.
Core file operations (open, close, sync, msync, truncate) are
now allowed to update the measurement immediately. In order
to maintain sufficient write performance for writes, add a
latency tunable delayed work workqueue for computing the
re-measurements.
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
include/linux/ima.h | 12 +++
security/integrity/ima/Kconfig | 20 +++++
security/integrity/ima/ima_appraise.c | 6 +-
security/integrity/ima/ima_main.c | 103 +++++++++++++++++++++++++-
security/integrity/integrity.h | 6 ++
5 files changed, 145 insertions(+), 2 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..c4e83f5c450a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,6 +20,8 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask);
extern void ima_post_create_tmpfile(struct inode *inode);
extern void ima_file_free(struct file *file);
+extern void ima_file_update(struct file *file);
+extern void ima_delayed_update(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
@@ -66,6 +68,16 @@ static inline void ima_file_free(struct file *file)
return;
}
+static inline void ima_file_update(struct file *file)
+{
+ return;
+}
+
+static inline void ima_delayed_update(struct file *file)
+{
+ return;
+}
+
static inline int ima_file_mmap(struct file *file, unsigned long prot)
{
return 0;
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..a2588d72cbc1 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -295,3 +295,23 @@ config IMA_APPRAISE_SIGNED_INIT
default n
help
This option requires user-space init to be signed.
+
+config IMA_HASH_LATENCY
+ int
+ depends on IMA_APPRAISE
+ range 0 60000
+ default 50
+ help
+ This value defines the re-measurement interval when files are
+ being written. Value is in milliseconds.
+
+config IMA_HASH_LATENCY_CEILING
+ int
+ depends on IMA_APPRAISE
+ range 100 60000
+ default 30000
+ help
+ In order to maintain high write performance for large files,
+ IMA increases the re-measurement rate as the file size grows.
+ This value defines the ceiling for the re-measurement delay
+ in milliseconds.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..9558ae0cc462 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -234,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_NOLABEL;
if (file->f_mode & FMODE_CREATED)
iint->flags |= IMA_NEW_FILE;
+
if ((iint->flags & IMA_NEW_FILE) &&
(!(iint->flags & IMA_DIGSIG_REQUIRED) ||
- (inode->i_size == 0)))
+ (inode->i_size == 0))) {
+ ima_fix_xattr(dentry, iint);
+ xattr_len = ima_read_xattr(dentry, &xattr_value);
status = INTEGRITY_PASS;
+ }
goto out;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..67d41005cd22 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -16,7 +16,7 @@
*
* File: ima_main.c
* implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- * and ima_file_check.
+ * ima_delayed_update, ima_file_update and ima_file_check.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -30,6 +30,8 @@
#include <linux/xattr.h>
#include <linux/ima.h>
#include <linux/iversion.h>
+#include <linux/workqueue.h>
+#include <linux/sizes.h>
#include <linux/fs.h>
#include "ima.h"
@@ -40,8 +42,15 @@ int ima_appraise = IMA_APPRAISE_ENFORCE;
int ima_appraise;
#endif
+#if CONFIG_IMA_HASH_LATENCY == 0
+#define IMA_LATENCY_INCREMENT 100
+#else
+#define IMA_LATENCY_INCREMENT CONFIG_IMA_HASH_LATENCY
+#endif
+
int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
+static struct workqueue_struct *ima_update_wq;
static int __init hash_setup(char *str)
{
@@ -127,6 +136,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
{
fmode_t mode = file->f_mode;
bool update;
+ bool creq;
if (!(mode & FMODE_WRITE))
return;
@@ -322,6 +332,17 @@ static int process_measurement(struct file *file, const struct cred *cred,
return 0;
}
+static void ima_delayed_update_handler(struct work_struct *work)
+{
+ struct ima_work_entry *entry;
+
+ entry = container_of(work, typeof(*entry), work.work);
+
+ ima_file_update(entry->file);
+ fput(entry->file);
+ entry->file = NULL;
+}
+
/**
* ima_file_mmap - based on policy, collect/store measurement.
* @file: pointer to the file to be measured (May be NULL)
@@ -375,6 +396,78 @@ int ima_bprm_check(struct linux_binprm *bprm)
MAY_EXEC, CREDS_CHECK);
}
+/**
+ * ima_delayed_update - add a file to delayed update list
+ * @file: pointer to file structure being updated
+ */
+void ima_delayed_update(struct file *file)
+{
+ struct inode *inode = file_inode(file);
+ struct integrity_iint_cache *iint;
+ unsigned long blocks;
+ unsigned long msecs;
+ bool creq;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ if (iint->ima_work.file)
+ return;
+
+ /* Slow down the samping rate per the file size */
+ blocks = inode->i_size / SZ_1M + 1;
+ msecs = blocks * IMA_LATENCY_INCREMENT;
+ if (msecs > CONFIG_IMA_HASH_LATENCY_CEILING)
+ msecs = CONFIG_IMA_HASH_LATENCY_CEILING;
+
+ get_file(file);
+ iint->ima_work.file = file;
+ INIT_DELAYED_WORK(&iint->ima_work.work, ima_delayed_update_handler);
+
+ creq = queue_delayed_work(ima_update_wq,
+ &iint->ima_work.work,
+ msecs_to_jiffies(msecs));
+ if (creq == false) {
+ iint->ima_work.file = NULL;
+ fput(file);
+ }
+}
+EXPORT_SYMBOL_GPL(ima_delayed_update);
+
+/**
+ * ima_file_update - update the file measurement
+ * @file: pointer to file structure being updated
+ */
+void ima_file_update(struct file *file)
+{
+ struct inode *inode = file_inode(file);
+ struct integrity_iint_cache *iint;
+ bool should_measure = true;
+ u64 i_version;
+
+ if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ return;
+
+ iint = integrity_iint_find(inode);
+ if (!iint)
+ return;
+
+ mutex_lock(&iint->mutex);
+ clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+ if (IS_I_VERSION(inode)) {
+ i_version = inode_query_iversion(inode);
+ if (i_version == iint->version)
+ should_measure = false;
+ }
+ if (should_measure) {
+ iint->flags &= ~IMA_COLLECTED;
+ ima_update_xattr(iint, file);
+ }
+ mutex_unlock(&iint->mutex);
+}
+EXPORT_SYMBOL_GPL(ima_file_update);
+
/**
* ima_path_check - based on policy, collect/store measurement.
* @file: pointer to the file to be measured
@@ -580,6 +673,12 @@ static int __init init_ima(void)
{
int error;
+ ima_update_wq = alloc_workqueue("ima-update-wq",
+ WQ_MEM_RECLAIM | WQ_CPU_INTENSIVE,
+ 0);
+ if (!ima_update_wq)
+ return -ENOMEM;
+
ima_init_template_list();
hash_setup(CONFIG_IMA_DEFAULT_HASH);
error = ima_init();
@@ -595,6 +694,8 @@ static int __init init_ima(void)
if (!error)
ima_update_policy_flag();
+ else
+ destroy_workqueue(ima_update_wq);
return error;
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7de59f44cba3..860c7a475a24 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -113,6 +113,11 @@ struct signature_v2_hdr {
uint8_t sig[0]; /* signature payload */
} __packed;
+struct ima_work_entry {
+ struct delayed_work work;
+ struct file *file;
+};
+
/* integrity data associated with an inode */
struct integrity_iint_cache {
struct rb_node rb_node; /* rooted in integrity_iint_tree */
@@ -129,6 +134,7 @@ struct integrity_iint_cache {
enum integrity_status ima_creds_status:4;
enum integrity_status evm_status:4;
struct ima_digest_data *ima_hash;
+ struct ima_work_entry ima_work;
};
/* rbtree tree calls to lookup, insert, delete
--
2.17.1
^ permalink raw reply related
* [PATCH 3/5] integrity: update the file measurement on write
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190513125354.23126-1-janne.karhunen@gmail.com>
When a file is being written, mark the file for IMA for delayed
re-measurement.
Depends on commit c8213962517e ("integrity: keep the integrity state of open files up to date")'
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
fs/read_write.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 177ccc3d405a..bfe10d6dc135 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -20,6 +20,7 @@
#include <linux/compat.h>
#include <linux/mount.h>
#include <linux/fs.h>
+#include <linux/ima.h>
#include "internal.h"
#include <linux/uaccess.h>
@@ -481,12 +482,18 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
static ssize_t __vfs_write(struct file *file, const char __user *p,
size_t count, loff_t *pos)
{
+ ssize_t sz;
+
if (file->f_op->write)
- return file->f_op->write(file, p, count, pos);
+ sz = file->f_op->write(file, p, count, pos);
else if (file->f_op->write_iter)
- return new_sync_write(file, p, count, pos);
+ sz = new_sync_write(file, p, count, pos);
else
return -EINVAL;
+
+ if (sz >= 1)
+ ima_delayed_update(file);
+ return sz;
}
ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t *pos)
--
2.17.1
^ permalink raw reply related
* [PATCH 4/5] integrity: measure the file on sync
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190513125354.23126-1-janne.karhunen@gmail.com>
When user requests a file sync, make sure that the IMA
measurement reflects the state of the data being sync'd.
Depends on commit c8213962517e ("integrity: keep the integrity state of open files up to date")'
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
fs/sync.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/sync.c b/fs/sync.c
index b54e0541ad89..90353f9ed931 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -16,6 +16,7 @@
#include <linux/pagemap.h>
#include <linux/quotaops.h>
#include <linux/backing-dev.h>
+#include <linux/ima.h>
#include "internal.h"
#define VALID_FLAGS (SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE| \
@@ -194,6 +195,8 @@ int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int datasync)
return -EINVAL;
if (!datasync && (inode->i_state & I_DIRTY_TIME))
mark_inode_dirty_sync(inode);
+ ima_file_update(file);
+
return file->f_op->fsync(file, start, end, datasync);
}
EXPORT_SYMBOL(vfs_fsync_range);
--
2.17.1
^ permalink raw reply related
* [PATCH 5/5] integrity: measure the file on msync
From: Janne Karhunen @ 2019-05-13 12:53 UTC (permalink / raw)
To: linux-integrity, linux-security-module, zohar
Cc: Janne Karhunen, Konsta Karsisto
In-Reply-To: <20190513125354.23126-1-janne.karhunen@gmail.com>
When user requests a file msync, make sure that the IMA
measurement reflects the state of the data being sync'd.
Depends on commit c8213962517e ("integrity: keep the integrity state of open files up to date")'
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
mm/msync.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/mm/msync.c b/mm/msync.c
index ef30a429623a..5a256e08b49d 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -14,6 +14,7 @@
#include <linux/file.h>
#include <linux/syscalls.h>
#include <linux/sched.h>
+#include <linux/ima.h>
/*
* MS_SYNC syncs the entire file - including mappings.
@@ -88,12 +89,18 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
get_file(file);
up_read(&mm->mmap_sem);
error = vfs_fsync_range(file, fstart, fend, 1);
+ if (!error)
+ ima_file_update(file);
+
fput(file);
if (error || start >= end)
goto out;
down_read(&mm->mmap_sem);
vma = find_vma(mm, start);
} else {
+ if (file)
+ ima_delayed_update(file);
+
if (start >= end) {
error = 0;
goto out_unlock;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 3/3] initramfs: introduce do_readxattrs()
From: Roberto Sassu @ 2019-05-13 13:03 UTC (permalink / raw)
To: Jann Horn
Cc: viro, linux-security-module, linux-integrity, initramfs,
linux-api, linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan
In-Reply-To: <20190510213340.GE253532@google.com>
On 5/10/2019 11:33 PM, Jann Horn wrote:
> On Thu, May 09, 2019 at 01:24:20PM +0200, Roberto Sassu wrote:
>> This patch adds support for an alternative method to add xattrs to files in
>> the rootfs filesystem. Instead of extracting them directly from the ram
>> disk image, they are extracted from a regular file called .xattr-list, that
>> can be added by any ram disk generator available today.
> [...]
>> +struct path_hdr {
>> + char p_size[10]; /* total size including p_size field */
>> + char p_data[]; /* <path>\0<xattrs> */
>> +};
>> +
>> +static int __init do_readxattrs(void)
>> +{
>> + struct path_hdr hdr;
>> + char str[sizeof(hdr.p_size) + 1];
>> + unsigned long file_entry_size;
>> + size_t size, name_buf_size, total_size;
>> + struct kstat st;
>> + int ret, fd;
>> +
>> + ret = vfs_lstat(XATTR_LIST_FILENAME, &st);
>> + if (ret < 0)
>> + return ret;
>> +
>> + total_size = st.size;
>> +
>> + fd = ksys_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
>> + if (fd < 0)
>> + return fd;
>> +
>> + while (total_size) {
>> + size = ksys_read(fd, (char *)&hdr, sizeof(hdr));
> [...]
>> + ksys_close(fd);
>> +
>> + if (ret < 0)
>> + error("Unable to parse xattrs");
>> +
>> + return ret;
>> +}
>
> Please use something like filp_open()+kernel_read()+fput() instead of
> ksys_open()+ksys_read()+ksys_close(). I understand that some of the init
> code needs to use the syscall wrappers because no equivalent VFS
> functions are available, but please use the VFS functions when that's
> easy to do.
Ok. Thanks for the suggestion.
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [GIT PULL] security subsystem: Tomoyo updates for v5.2
From: James Morris @ 2019-05-13 17:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LSM List, Linux List Kernel Mailing
In-Reply-To: <CAHk-=wg8UFHD_KmTWF3LMnDf_VN7cv_pofpc4eOHmx_8kmMPWw@mail.gmail.com>
On Sat, 11 May 2019, Linus Torvalds wrote:
> On Fri, May 10, 2019 at 6:09 PM James Morris <jmorris@namei.org> wrote:
> >
> > These patches include fixes to enable fuzz testing, and a fix for
> > calculating whether a filesystem is user-modifiable.
>
> So now these have been very recently rebased (on top of a random
> merge-window "tree of the day" version) instead of having multiple
> merges.
>
> That makes the history cleaner, but has its own issues.
These are just plain patches from the author, they were not part of any
publicly accessible tree -- there's nowhere they could be merged from.
> We really need to find a different model for the security layer patches.
>
> Linus
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [GIT PULL] security subsystem: Tomoyo updates for v5.2
From: James Morris @ 2019-05-13 18:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Paul Moore, Casey Schaufler, LSM List, Linux List Kernel Mailing
In-Reply-To: <CAHk-=wh33PsgxtwXdzDgx13tARBnhzkPV9294L2g6OjOBeNvfA@mail.gmail.com>
On Sun, 12 May 2019, Linus Torvalds wrote:
> > My guess is that you are right and
> > any *significant* changes to the LSM layer itself, e.g. security/*, is
> > best sent via James' tree. For smaller changes to the LSM layer I
> > think it's okay if they go in via an individual LSM tree so long as
> > all the other LSMs agree-on/ack the changes; which pretty much fits
> > what we've been doing for some time now and it seems to work well
> > enough.
>
> Yeah, I think that's the sane model. And I think it's mostly been working.
New LSMs also need to be guided in, as part of a community effort.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [GIT PULL] security subsystem: Tomoyo updates for v5.2
From: James Morris @ 2019-05-13 18:22 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LSM List, Linux List Kernel Mailing
In-Reply-To: <alpine.LRH.2.21.1905140352370.14684@namei.org>
On Tue, 14 May 2019, James Morris wrote:
> On Sat, 11 May 2019, Linus Torvalds wrote:
> > So now these have been very recently rebased (on top of a random
> > merge-window "tree of the day" version) instead of having multiple
> > merges.
> >
> > That makes the history cleaner, but has its own issues.
>
> These are just plain patches from the author, they were not part of any
> publicly accessible tree -- there's nowhere they could be merged from.
The reason I created a new branch in this case was that the existing one
had the same issue as the TPM branch.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v10 09/12] ima: Implement support for module-style appended signatures
From: Mimi Zohar @ 2019-05-14 12:09 UTC (permalink / raw)
To: Thiago Jung Bauermann, linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, Jonathan Corbet, AKASHI, Takahiro
In-Reply-To: <20190418035120.2354-10-bauerman@linux.ibm.com>
Hi Thiago,
On Thu, 2019-04-18 at 00:51 -0300, Thiago Jung Bauermann wrote:
>
> @@ -326,6 +356,10 @@ int ima_appraise_measurement(enum ima_hooks func,
> case INTEGRITY_UNKNOWN:
> break;
> case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
> + /* It's fine not to have xattrs when using a modsig. */
> + if (try_modsig)
> + break;
> + /* fall through */
> case INTEGRITY_NOLABEL: /* No security.evm xattr. */
> cause = "missing-HMAC";
> goto out;
> @@ -340,6 +374,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> rc = xattr_verify(func, iint, xattr_value, xattr_len, &status,
> &cause);
>
> + /*
> + * If we have a modsig and either no imasig or the imasig's key isn't
> + * known, then try verifying the modsig.
> + */
> + if (status != INTEGRITY_PASS && try_modsig &&
> + (!xattr_value || rc == -ENOKEY))
> + rc = modsig_verify(func, modsig, &status, &cause);
EVM protects other security xattrs, not just security.ima, if they
exist. As a result, evm_verifyxattr() could pass based on the other
security xattrs.
Mimi
> +
> out:
> /*
> * File signatures on some filesystems can not be properly verified.
^ permalink raw reply
* [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-14 14:35 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland, linux-mm,
linux-security-module
In-Reply-To: <20190514143537.10435-1-glider@google.com>
The new options are needed to prevent possible information leaks and
make control-flow bugs that depend on uninitialized values more
deterministic.
init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
objects with zeroes. Initialization is done at allocation time at the
places where checks for __GFP_ZERO are performed.
init_on_free=1 makes the kernel initialize freed pages and heap objects
with zeroes upon their deletion. This helps to ensure sensitive data
doesn't leak via use-after-free accesses.
Both init_on_alloc=1 and init_on_free=1 guarantee that the allocator
returns zeroed memory. The only exception is slab caches with
constructors. Those are never zero-initialized to preserve their semantics.
For SLOB allocator init_on_free=1 also implies init_on_alloc=1 behavior,
i.e. objects are zeroed at both allocation and deallocation time.
This is done because SLOB may otherwise return multiple freelist pointers
in the allocated object. For SLAB and SLUB enabling either init_on_alloc
or init_on_free leads to one-time initialization of the object.
Both init_on_alloc and init_on_free default to zero, but those defaults
can be overridden with CONFIG_INIT_ON_ALLOC_DEFAULT_ON and
CONFIG_INIT_ON_FREE_DEFAULT_ON.
Slowdown for the new features compared to init_on_free=0,
init_on_alloc=0:
hackbench, init_on_free=1: +7.62% sys time (st.err 0.74%)
hackbench, init_on_alloc=1: +7.75% sys time (st.err 2.14%)
Linux build with -j12, init_on_free=1: +8.38% wall time (st.err 0.39%)
Linux build with -j12, init_on_free=1: +24.42% sys time (st.err 0.52%)
Linux build with -j12, init_on_alloc=1: -0.13% wall time (st.err 0.42%)
Linux build with -j12, init_on_alloc=1: +0.57% sys time (st.err 0.40%)
The slowdown for init_on_free=0, init_on_alloc=0 compared to the
baseline is within the standard error.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
To: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- unconditionally initialize pages in kernel_init_free_pages()
- comment from Randy Dunlap: drop 'default false' lines from Kconfig.hardening
---
.../admin-guide/kernel-parameters.txt | 8 +++
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 22 ++++++
kernel/kexec_core.c | 2 +-
mm/dmapool.c | 2 +-
mm/page_alloc.c | 68 ++++++++++++++++---
mm/slab.c | 16 ++++-
mm/slab.h | 16 +++++
mm/slob.c | 22 +++++-
mm/slub.c | 27 ++++++--
net/core/sock.c | 2 +-
security/Kconfig.hardening | 14 ++++
12 files changed, 178 insertions(+), 23 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 08df58805703..cece9a56ddb1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1673,6 +1673,14 @@
initrd= [BOOT] Specify the location of the initial ramdisk
+ init_on_alloc= [MM] Fill newly allocated pages and heap objects with
+ zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
+ init_on_free= [MM] Fill freed pages and heap objects with zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_FREE_DEFAULT_ON.
+
init_pkru= [x86] Specify the default memory protection keys rights
register contents for all processes. 0x55555554 by
default (disallow access to all but pkey 0). Can
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 829b0c6944d8..61758201d9b2 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -127,7 +127,7 @@ __malloc void *_uverbs_alloc(struct uverbs_attr_bundle *bundle, size_t size,
res = (void *)pbundle->internal_buffer + pbundle->internal_used;
pbundle->internal_used =
ALIGN(new_used, sizeof(*pbundle->internal_buffer));
- if (flags & __GFP_ZERO)
+ if (want_init_on_alloc(flags))
memset(res, 0, size);
return res;
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 083d7b4863ed..18d96f1d07c5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2610,6 +2610,28 @@ static inline void kernel_poison_pages(struct page *page, int numpages,
int enable) { }
#endif
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+static inline bool want_init_on_alloc(gfp_t flags)
+{
+ if (static_branch_unlikely(&init_on_alloc))
+ return true;
+ return flags & __GFP_ZERO;
+}
+
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_free);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_free);
+#endif
+static inline bool want_init_on_free(void)
+{
+ return static_branch_unlikely(&init_on_free);
+}
+
extern bool _debug_pagealloc_enabled;
static inline bool debug_pagealloc_enabled(void)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index fd5c95ff9251..2f75dd0d0d81 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
arch_kexec_post_alloc_pages(page_address(pages), count,
gfp_mask);
- if (gfp_mask & __GFP_ZERO)
+ if (want_init_on_alloc(gfp_mask))
for (i = 0; i < count; i++)
clear_highpage(pages + i);
}
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 76a160083506..493d151067cb 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -381,7 +381,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
#endif
spin_unlock_irqrestore(&pool->lock, flags);
- if (mem_flags & __GFP_ZERO)
+ if (want_init_on_alloc(mem_flags))
memset(retval, 0, pool->size);
return retval;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 59661106da16..463c681a3633 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -133,6 +133,48 @@ unsigned long totalcma_pages __read_mostly;
int percpu_pagelist_fraction;
gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_free);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_free);
+#endif
+
+static int __init early_init_on_alloc(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_alloc);
+ else
+ static_branch_disable(&init_on_alloc);
+ return ret;
+}
+early_param("init_on_alloc", early_init_on_alloc);
+
+static int __init early_init_on_free(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_free);
+ else
+ static_branch_disable(&init_on_free);
+ return ret;
+}
+early_param("init_on_free", early_init_on_free);
/*
* A cached value of the page's pageblock's migratetype, used when the page is
@@ -1092,6 +1134,14 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
return ret;
}
+static void kernel_init_free_pages(struct page *page, int numpages)
+{
+ int i;
+
+ for (i = 0; i < numpages; i++)
+ clear_highpage(page + i);
+}
+
static __always_inline bool free_pages_prepare(struct page *page,
unsigned int order, bool check_free)
{
@@ -1144,9 +1194,10 @@ static __always_inline bool free_pages_prepare(struct page *page,
}
arch_free_page(page, order);
kernel_poison_pages(page, 1 << order, 0);
+ if (want_init_on_free())
+ kernel_init_free_pages(page, 1 << order);
if (debug_pagealloc_enabled())
kernel_map_pages(page, 1 << order, 0);
-
kasan_free_nondeferred_pages(page, order);
return true;
@@ -1452,8 +1503,10 @@ meminit_pfn_in_nid(unsigned long pfn, int node,
void __init memblock_free_pages(struct page *page, unsigned long pfn,
unsigned int order)
{
- if (early_page_uninitialised(pfn))
+ if (early_page_uninitialised(pfn)) {
+ kernel_init_free_pages(page, 1 << order);
return;
+ }
__free_pages_core(page, order);
}
@@ -1971,8 +2024,8 @@ static inline int check_new_page(struct page *page)
static inline bool free_pages_prezeroed(void)
{
- return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
- page_poisoning_enabled();
+ return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
+ page_poisoning_enabled()) || want_init_on_free();
}
#ifdef CONFIG_DEBUG_VM
@@ -2026,13 +2079,10 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
unsigned int alloc_flags)
{
- int i;
-
post_alloc_hook(page, order, gfp_flags);
- if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
- for (i = 0; i < (1 << order); i++)
- clear_highpage(page + i);
+ if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
+ kernel_init_free_pages(page, 1 << order);
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
diff --git a/mm/slab.c b/mm/slab.c
index 284ab737faee..d00e9de26a45 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1855,6 +1855,14 @@ static bool set_objfreelist_slab_cache(struct kmem_cache *cachep,
cachep->num = 0;
+ /*
+ * If slab auto-initialization on free is enabled, store the freelist
+ * off-slab, so that its contents don't end up in one of the allocated
+ * objects.
+ */
+ if (unlikely(slab_want_init_on_free(cachep)))
+ return false;
+
if (cachep->ctor || flags & SLAB_TYPESAFE_BY_RCU)
return false;
@@ -3294,7 +3302,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
local_irq_restore(save_flags);
ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
- if (unlikely(flags & __GFP_ZERO) && ptr)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
memset(ptr, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &ptr);
@@ -3351,7 +3359,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
prefetchw(objp);
- if (unlikely(flags & __GFP_ZERO) && objp)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
memset(objp, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &objp);
@@ -3472,6 +3480,8 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
struct array_cache *ac = cpu_cache_get(cachep);
check_irq_off();
+ if (unlikely(slab_want_init_on_free(cachep)))
+ memset(objp, 0, cachep->object_size);
kmemleak_free_recursive(objp, cachep->flags);
objp = cache_free_debugcheck(cachep, objp, caller);
@@ -3559,7 +3569,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
cache_alloc_debugcheck_after_bulk(s, flags, size, p, _RET_IP_);
/* Clear memory outside IRQ disabled section */
- if (unlikely(flags & __GFP_ZERO))
+ if (unlikely(slab_want_init_on_alloc(flags, s)))
for (i = 0; i < size; i++)
memset(p[i], 0, s->object_size);
diff --git a/mm/slab.h b/mm/slab.h
index 43ac818b8592..24ae887359b8 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -524,4 +524,20 @@ static inline int cache_random_seq_create(struct kmem_cache *cachep,
static inline void cache_random_seq_destroy(struct kmem_cache *cachep) { }
#endif /* CONFIG_SLAB_FREELIST_RANDOM */
+static inline bool slab_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_alloc))
+ return !(c->ctor);
+ else
+ return flags & __GFP_ZERO;
+}
+
+static inline bool slab_want_init_on_free(struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_free))
+ return !(c->ctor);
+ else
+ return false;
+}
+
#endif /* MM_SLAB_H */
diff --git a/mm/slob.c b/mm/slob.c
index 307c2c9feb44..351d3dfee000 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -212,6 +212,19 @@ static void slob_free_pages(void *b, int order)
free_pages((unsigned long)b, order);
}
+/*
+ * init_on_free=1 also implies initialization at allocation time.
+ * This is because newly allocated objects may contain freelist pointers
+ * somewhere in the middle.
+ */
+static inline bool slob_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_alloc) ||
+ static_branch_unlikely(&init_on_free))
+ return c ? (!c->ctor) : true;
+ return flags & __GFP_ZERO;
+}
+
/*
* Allocate a slob block within a given slob_page sp.
*/
@@ -330,8 +343,6 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node)
BUG_ON(!b);
spin_unlock_irqrestore(&slob_lock, flags);
}
- if (unlikely(gfp & __GFP_ZERO))
- memset(b, 0, size);
return b;
}
@@ -366,6 +377,9 @@ static void slob_free(void *block, int size)
return;
}
+ if (unlikely(want_init_on_free()))
+ memset(block, 0, size);
+
if (!slob_page_free(sp)) {
/* This slob page is about to become partially free. Easy! */
sp->units = units;
@@ -461,6 +475,8 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
}
kmemleak_alloc(ret, size, 1, gfp);
+ if (unlikely(slob_want_init_on_alloc(gfp, 0)))
+ memset(ret, 0, size);
return ret;
}
@@ -559,6 +575,8 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
WARN_ON_ONCE(flags & __GFP_ZERO);
c->ctor(b);
}
+ if (unlikely(slob_want_init_on_alloc(flags, c)))
+ memset(b, 0, c->size);
kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
return b;
diff --git a/mm/slub.c b/mm/slub.c
index 6b28cd2b5a58..01424e910800 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1423,6 +1423,19 @@ static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
static inline bool slab_free_freelist_hook(struct kmem_cache *s,
void **head, void **tail)
{
+
+ void *object;
+ void *next = *head;
+ void *old_tail = *tail ? *tail : *head;
+
+ if (slab_want_init_on_free(s))
+ do {
+ object = next;
+ next = get_freepointer(s, object);
+ memset(object, 0, s->size);
+ set_freepointer(s, object, next);
+ } while (object != old_tail);
+
/*
* Compiler cannot detect this function can be removed if slab_free_hook()
* evaluates to nothing. Thus, catch all relevant config debug options here.
@@ -1432,9 +1445,7 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
defined(CONFIG_DEBUG_OBJECTS_FREE) || \
defined(CONFIG_KASAN)
- void *object;
- void *next = *head;
- void *old_tail = *tail ? *tail : *head;
+ next = *head;
/* Head and tail of the reconstructed freelist */
*head = NULL;
@@ -2740,8 +2751,14 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
prefetch_freepointer(s, next_object);
stat(s, ALLOC_FASTPATH);
}
+ /*
+ * If the object has been wiped upon free, make sure it's fully
+ * initialized by zeroing out freelist pointer.
+ */
+ if (slab_want_init_on_free(s))
+ *(void **)object = 0;
- if (unlikely(gfpflags & __GFP_ZERO) && object)
+ if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
memset(object, 0, s->object_size);
slab_post_alloc_hook(s, gfpflags, 1, &object);
@@ -3163,7 +3180,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
local_irq_enable();
/* Clear memory outside IRQ disabled fastpath loop */
- if (unlikely(flags & __GFP_ZERO)) {
+ if (unlikely(slab_want_init_on_alloc(flags, s))) {
int j;
for (j = 0; j < i; j++)
diff --git a/net/core/sock.c b/net/core/sock.c
index 75b1c950b49f..9ceb90c875bc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1602,7 +1602,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
if (!sk)
return sk;
- if (priority & __GFP_ZERO)
+ if (want_init_on_alloc(priority))
sk_prot_clear_nulls(sk, prot->obj_size);
} else
sk = kmalloc(prot->obj_size, priority);
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 0a1d4ca314f4..87883e3e3c2a 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -159,6 +159,20 @@ config STACKLEAK_RUNTIME_DISABLE
runtime to control kernel stack erasing for kernels built with
CONFIG_GCC_PLUGIN_STACKLEAK.
+config INIT_ON_ALLOC_DEFAULT_ON
+ bool "Set init_on_alloc=1 by default"
+ help
+ Enable init_on_alloc=1 by default, making the kernel initialize every
+ page and heap allocation with zeroes.
+ init_on_alloc can be overridden via command line.
+
+config INIT_ON_FREE_DEFAULT_ON
+ bool "Set init_on_free=1 by default"
+ help
+ Enable init_on_free=1 by default, making the kernel initialize freed
+ pages and slab memory with zeroes.
+ init_on_free can be overridden via command line.
+
endmenu
endmenu
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH v2 2/4] lib: introduce test_meminit module
From: Alexander Potapenko @ 2019-05-14 14:35 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Jann Horn, linux-mm,
linux-security-module
In-Reply-To: <20190514143537.10435-1-glider@google.com>
Add tests for heap and pagealloc initialization.
These can be used to check init_on_alloc and init_on_free implementations
as well as other approaches to initialization.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
lib/Kconfig.debug | 8 ++
lib/Makefile | 1 +
lib/test_meminit.c | 205 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 214 insertions(+)
create mode 100644 lib/test_meminit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d695ec1477f3..6c3fc68a4a77 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2020,6 +2020,14 @@ config TEST_STACKINIT
If unsure, say N.
+config TEST_MEMINIT
+ tristate "Test level of heap/page initialization"
+ help
+ Test if the kernel is zero-initializing heap and page allocations.
+ This can be useful to test init_on_alloc and init_on_free features.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index 83d7df2661ff..29c5afbe9882 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
+obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
diff --git a/lib/test_meminit.c b/lib/test_meminit.c
new file mode 100644
index 000000000000..67d759498030
--- /dev/null
+++ b/lib/test_meminit.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for SL[AOU]B/page initialization at alloc/free time.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#define GARBAGE_INT (0x09A7BA9E)
+#define GARBAGE_BYTE (0x9E)
+
+#define REPORT_FAILURES_IN_FN() \
+ do { \
+ if (failures) \
+ pr_info("%s failed %d out of %d times\n", \
+ __func__, failures, num_tests); \
+ else \
+ pr_info("all %d tests in %s passed\n", \
+ num_tests, __func__); \
+ } while (0)
+
+/* Calculate the number of uninitialized bytes in the buffer. */
+static int count_nonzero_bytes(void *ptr, size_t size)
+{
+ int i, ret = 0;
+ unsigned char *p = (unsigned char *)ptr;
+
+ for (i = 0; i < size; i++)
+ if (p[i])
+ ret++;
+ return ret;
+}
+
+static void fill_with_garbage(void *ptr, size_t size)
+{
+ unsigned int *p = (unsigned int *)ptr;
+ int i = 0;
+
+ while (size >= sizeof(*p)) {
+ p[i] = GARBAGE_INT;
+ i++;
+ size -= sizeof(*p);
+ }
+ if (size)
+ memset(&p[i], GARBAGE_BYTE, size);
+}
+
+static int __init do_alloc_pages_order(int order, int *total_failures)
+{
+ struct page *page;
+ void *buf;
+ size_t size = PAGE_SIZE << order;
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+ return 1;
+}
+
+static int __init test_pages(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i;
+
+ for (i = 0; i < 10; i++)
+ num_tests += do_alloc_pages_order(i, &failures);
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+static int __init do_kmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = kmalloc(size, GFP_KERNEL);
+ fill_with_garbage(buf, size);
+ kfree(buf);
+
+ buf = kmalloc(size, GFP_KERNEL);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ kfree(buf);
+ return 1;
+}
+
+static int __init do_vmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = vmalloc(size);
+ fill_with_garbage(buf, size);
+ vfree(buf);
+
+ buf = vmalloc(size);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ vfree(buf);
+ return 1;
+}
+
+static int __init test_kvmalloc(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 20; i++) {
+ size = 1 << i;
+ num_tests += do_kmalloc_size(size, &failures);
+ num_tests += do_vmalloc_size(size, &failures);
+ }
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+#define CTOR_BYTES 4
+/* Initialize the first 4 bytes of the object. */
+void some_ctor(void *obj)
+{
+ memset(obj, 'A', CTOR_BYTES);
+}
+
+static int __init do_kmem_cache_size(size_t size, bool want_ctor,
+ int *total_failures)
+{
+ struct kmem_cache *c;
+ void *buf;
+ int iter, bytes = 0;
+ int fail = 0;
+
+ c = kmem_cache_create("test_cache", size, 1, 0,
+ want_ctor ? some_ctor : NULL);
+ for (iter = 0; iter < 10; iter++) {
+ buf = kmem_cache_alloc(c, GFP_KERNEL);
+ if (!want_ctor || iter == 0)
+ bytes = count_nonzero_bytes(buf, size);
+ if (want_ctor) {
+ /*
+ * Newly initialized memory must be initialized using
+ * the constructor.
+ */
+ if (iter == 0 && bytes < CTOR_BYTES)
+ fail = 1;
+ } else {
+ if (bytes)
+ fail = 1;
+ }
+ fill_with_garbage(buf, size);
+ kmem_cache_free(c, buf);
+ }
+ kmem_cache_destroy(c);
+
+ *total_failures += fail;
+ return 1;
+}
+
+static int __init test_kmemcache(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 10; i++) {
+ size = 4 << i;
+ num_tests += do_kmem_cache_size(size, false, &failures);
+ num_tests += do_kmem_cache_size(size, true, &failures);
+ }
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+static int __init test_meminit_init(void)
+{
+ int failures = 0, num_tests = 0;
+
+ num_tests += test_pages(&failures);
+ num_tests += test_kvmalloc(&failures);
+ num_tests += test_kmemcache(&failures);
+
+ if (failures == 0)
+ pr_info("all %d tests passed!\n", num_tests);
+ else
+ pr_info("failures: %d out of %d\n", failures, num_tests);
+
+ return failures ? -EINVAL : 0;
+}
+module_init(test_meminit_init);
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH v2 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Alexander Potapenko @ 2019-05-14 14:35 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
Souptick Joarder, Matthew Wilcox, linux-mm, linux-security-module
In-Reply-To: <20190514143537.10435-1-glider@google.com>
When passed to an allocator (either pagealloc or SL[AOU]B),
__GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
init_on_alloc boot option is enabled. This can be useful in the cases
newly allocated memory is going to be initialized by the caller right
away.
__GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
where init_on_free implies init_on_alloc.
__GFP_NO_AUTOINIT basically defeats the hardening against information
leaks provided by init_on_alloc, so one should use it with caution.
This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
Doing so is safe, because the heap allocators initialize the pages they
receive before passing memory to the callers.
Slowdown for the initialization features compared to init_on_free=0,
init_on_alloc=0:
hackbench, init_on_free=1: +6.84% sys time (st.err 0.74%)
hackbench, init_on_alloc=1: +7.25% sys time (st.err 0.72%)
Linux build with -j12, init_on_free=1: +8.52% wall time (st.err 0.42%)
Linux build with -j12, init_on_free=1: +24.31% sys time (st.err 0.47%)
Linux build with -j12, init_on_alloc=1: -0.16% wall time (st.err 0.40%)
Linux build with -j12, init_on_alloc=1: +1.24% sys time (st.err 0.39%)
The slowdown for init_on_free=0, init_on_alloc=0 compared to the
baseline is within the standard error.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
To: Kees Cook <keescook@chromium.org>
To: Christoph Lameter <cl@linux.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- renamed __GFP_NOINIT to __GFP_NO_AUTOINIT, updated patch
name/description
---
include/linux/gfp.h | 13 +++++++++----
include/linux/mm.h | 2 +-
kernel/kexec_core.c | 3 ++-
mm/slab.c | 2 +-
mm/slob.c | 3 ++-
mm/slub.c | 1 +
6 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index fdab7de7490d..e1a83bd0ca67 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -44,6 +44,7 @@ struct vm_area_struct;
#else
#define ___GFP_NOLOCKDEP 0
#endif
+#define ___GFP_NO_AUTOINIT 0x1000000u
/* If the above are modified, __GFP_BITS_SHIFT may need updating */
/*
@@ -208,16 +209,20 @@ struct vm_area_struct;
* %__GFP_COMP address compound page metadata.
*
* %__GFP_ZERO returns a zeroed page on success.
+ *
+ * %__GFP_NO_AUTOINIT requests non-initialized memory from the underlying
+ * allocator.
*/
-#define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)
-#define __GFP_COMP ((__force gfp_t)___GFP_COMP)
-#define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)
+#define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)
+#define __GFP_COMP ((__force gfp_t)___GFP_COMP)
+#define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)
+#define __GFP_NO_AUTOINIT ((__force gfp_t)___GFP_NO_AUTOINIT)
/* Disable lockdep for GFP context tracking */
#define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
/* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
+#define __GFP_BITS_SHIFT (25)
#define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
/**
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 18d96f1d07c5..ce6c63396002 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2618,7 +2618,7 @@ DECLARE_STATIC_KEY_FALSE(init_on_alloc);
static inline bool want_init_on_alloc(gfp_t flags)
{
if (static_branch_unlikely(&init_on_alloc))
- return true;
+ return !(flags & __GFP_NO_AUTOINIT);
return flags & __GFP_ZERO;
}
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 2f75dd0d0d81..7fc37bacac79 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -302,7 +302,8 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
{
struct page *pages;
- pages = alloc_pages(gfp_mask & ~__GFP_ZERO, order);
+ pages = alloc_pages((gfp_mask & ~__GFP_ZERO) | __GFP_NO_AUTOINIT,
+ order);
if (pages) {
unsigned int count, i;
diff --git a/mm/slab.c b/mm/slab.c
index d00e9de26a45..1089461fc22b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1393,7 +1393,7 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
struct page *page;
int nr_pages;
- flags |= cachep->allocflags;
+ flags |= (cachep->allocflags | __GFP_NO_AUTOINIT);
page = __alloc_pages_node(nodeid, flags, cachep->gfporder);
if (!page) {
diff --git a/mm/slob.c b/mm/slob.c
index 351d3dfee000..d505f36aa398 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -192,6 +192,7 @@ static void *slob_new_pages(gfp_t gfp, int order, int node)
{
void *page;
+ gfp |= __GFP_NO_AUTOINIT;
#ifdef CONFIG_NUMA
if (node != NUMA_NO_NODE)
page = __alloc_pages_node(node, gfp, order);
@@ -221,7 +222,7 @@ static inline bool slob_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
{
if (static_branch_unlikely(&init_on_alloc) ||
static_branch_unlikely(&init_on_free))
- return c ? (!c->ctor) : true;
+ return c ? (!c->ctor) : !(flags & __GFP_NO_AUTOINIT);
return flags & __GFP_ZERO;
}
diff --git a/mm/slub.c b/mm/slub.c
index 01424e910800..0aa306f5769a 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1495,6 +1495,7 @@ static inline struct page *alloc_slab_page(struct kmem_cache *s,
struct page *page;
unsigned int order = oo_order(oo);
+ flags |= __GFP_NO_AUTOINIT;
if (node == NUMA_NO_NODE)
page = alloc_pages(flags, order);
else
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Alexander Potapenko @ 2019-05-14 14:35 UTC (permalink / raw)
To: akpm, cl, keescook
Cc: kernel-hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland, linux-mm,
linux-security-module
In-Reply-To: <20190514143537.10435-1-glider@google.com>
Add sock_alloc_send_pskb_noinit(), which is similar to
sock_alloc_send_pskb(), but allocates with __GFP_NO_AUTOINIT.
This helps reduce the slowdown on hackbench in the init_on_alloc mode
from 6.84% to 3.45%.
Slowdown for the initialization features compared to init_on_free=0,
init_on_alloc=0:
hackbench, init_on_free=1: +7.71% sys time (st.err 0.45%)
hackbench, init_on_alloc=1: +3.45% sys time (st.err 0.86%)
Linux build with -j12, init_on_free=1: +8.34% wall time (st.err 0.39%)
Linux build with -j12, init_on_free=1: +24.13% sys time (st.err 0.47%)
Linux build with -j12, init_on_alloc=1: -0.04% wall time (st.err 0.46%)
Linux build with -j12, init_on_alloc=1: +0.50% sys time (st.err 0.45%)
The slowdown for init_on_free=0, init_on_alloc=0 compared to the
baseline is within the standard error.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
To: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- changed __GFP_NOINIT to __GFP_NO_AUTOINIT
---
include/net/sock.h | 5 +++++
net/core/sock.c | 29 +++++++++++++++++++++++++----
net/unix/af_unix.c | 13 +++++++------
3 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 4d208c0f9c14..0dcb90a0c14d 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1626,6 +1626,11 @@ struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
unsigned long data_len, int noblock,
int *errcode, int max_page_order);
+struct sk_buff *sock_alloc_send_pskb_noinit(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order);
void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
void sock_kfree_s(struct sock *sk, void *mem, int size);
void sock_kzfree_s(struct sock *sk, void *mem, int size);
diff --git a/net/core/sock.c b/net/core/sock.c
index 9ceb90c875bc..7c24b70b7069 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2192,9 +2192,11 @@ static long sock_wait_for_wmem(struct sock *sk, long timeo)
* Generic send/receive buffer handlers
*/
-struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
- unsigned long data_len, int noblock,
- int *errcode, int max_page_order)
+struct sk_buff *sock_alloc_send_pskb_internal(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order, gfp_t gfp)
{
struct sk_buff *skb;
long timeo;
@@ -2223,7 +2225,7 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
timeo = sock_wait_for_wmem(sk, timeo);
}
skb = alloc_skb_with_frags(header_len, data_len, max_page_order,
- errcode, sk->sk_allocation);
+ errcode, sk->sk_allocation | gfp);
if (skb)
skb_set_owner_w(skb, sk);
return skb;
@@ -2234,8 +2236,27 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
*errcode = err;
return NULL;
}
+
+struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
+ unsigned long data_len, int noblock,
+ int *errcode, int max_page_order)
+{
+ return sock_alloc_send_pskb_internal(sk, header_len, data_len,
+ noblock, errcode, max_page_order, /*gfp*/0);
+}
EXPORT_SYMBOL(sock_alloc_send_pskb);
+struct sk_buff *sock_alloc_send_pskb_noinit(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order)
+{
+ return sock_alloc_send_pskb_internal(sk, header_len, data_len,
+ noblock, errcode, max_page_order, /*gfp*/__GFP_NO_AUTOINIT);
+}
+EXPORT_SYMBOL(sock_alloc_send_pskb_noinit);
+
struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
int noblock, int *errcode)
{
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e68d7454f2e3..a4c15620b66d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1627,9 +1627,9 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
BUILD_BUG_ON(SKB_MAX_ALLOC < PAGE_SIZE);
}
- skb = sock_alloc_send_pskb(sk, len - data_len, data_len,
- msg->msg_flags & MSG_DONTWAIT, &err,
- PAGE_ALLOC_COSTLY_ORDER);
+ skb = sock_alloc_send_pskb_noinit(sk, len - data_len, data_len,
+ msg->msg_flags & MSG_DONTWAIT, &err,
+ PAGE_ALLOC_COSTLY_ORDER);
if (skb == NULL)
goto out;
@@ -1824,9 +1824,10 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
data_len = min_t(size_t, size, PAGE_ALIGN(data_len));
- skb = sock_alloc_send_pskb(sk, size - data_len, data_len,
- msg->msg_flags & MSG_DONTWAIT, &err,
- get_order(UNIX_SKB_FRAGS_SZ));
+ skb = sock_alloc_send_pskb_noinit(sk, size - data_len, data_len,
+ msg->msg_flags & MSG_DONTWAIT,
+ &err,
+ get_order(UNIX_SKB_FRAGS_SZ));
if (!skb)
goto out_err;
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* Re: [PATCH 3/4] gfp: mm: introduce __GFP_NOINIT
From: Alexander Potapenko @ 2019-05-14 14:39 UTC (permalink / raw)
To: Souptick Joarder
Cc: Kees Cook, Andrew Morton, Christoph Lameter, Laura Abbott,
Linux-MM, linux-security-module, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Randy Dunlap,
Jann Horn, Mark Rutland, Matthew Wilcox
In-Reply-To: <CAFqt6zY1oY4YTfAd4RdV0-V8iUfK65LTHsdmxrSWs7KRnWrrCg@mail.gmail.com>
From: Souptick Joarder <jrdr.linux@gmail.com>
Date: Sat, May 11, 2019 at 9:28 AM
To: Alexander Potapenko
Cc: Kees Cook, Andrew Morton, Christoph Lameter, Laura Abbott,
Linux-MM, linux-security-module, Kernel Hardening, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Randy Dunlap, Jann Horn, Mark Rutland,
Matthew Wilcox
> On Thu, May 9, 2019 at 6:53 PM Alexander Potapenko <glider@google.com> wrote:
> >
> > From: Kees Cook <keescook@chromium.org>
> > Date: Wed, May 8, 2019 at 9:16 PM
> > To: Alexander Potapenko
> > Cc: Andrew Morton, Christoph Lameter, Kees Cook, Laura Abbott,
> > Linux-MM, linux-security-module, Kernel Hardening, Masahiro Yamada,
> > James Morris, Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
> > Dmitry Vyukov, Sandeep Patil, Randy Dunlap, Jann Horn, Mark Rutland
> >
> > > On Wed, May 8, 2019 at 8:38 AM Alexander Potapenko <glider@google.com> wrote:
> > > > When passed to an allocator (either pagealloc or SL[AOU]B), __GFP_NOINIT
> > > > tells it to not initialize the requested memory if the init_on_alloc
> > > > boot option is enabled. This can be useful in the cases newly allocated
> > > > memory is going to be initialized by the caller right away.
> > > >
> > > > __GFP_NOINIT doesn't affect init_on_free behavior, except for SLOB,
> > > > where init_on_free implies init_on_alloc.
> > > >
> > > > __GFP_NOINIT basically defeats the hardening against information leaks
> > > > provided by init_on_alloc, so one should use it with caution.
> > > >
> > > > This patch also adds __GFP_NOINIT to alloc_pages() calls in SL[AOU]B.
> > > > Doing so is safe, because the heap allocators initialize the pages they
> > > > receive before passing memory to the callers.
> > > >
> > > > Slowdown for the initialization features compared to init_on_free=0,
> > > > init_on_alloc=0:
> > > >
> > > > hackbench, init_on_free=1: +6.84% sys time (st.err 0.74%)
> > > > hackbench, init_on_alloc=1: +7.25% sys time (st.err 0.72%)
> > > >
> > > > Linux build with -j12, init_on_free=1: +8.52% wall time (st.err 0.42%)
> > > > Linux build with -j12, init_on_free=1: +24.31% sys time (st.err 0.47%)
> > > > Linux build with -j12, init_on_alloc=1: -0.16% wall time (st.err 0.40%)
> > > > Linux build with -j12, init_on_alloc=1: +1.24% sys time (st.err 0.39%)
> > > >
> > > > The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> > > > baseline is within the standard error.
> > > >
>
> Not sure, but I think this patch will clash with Matthew's posted patch series
> *Remove 'order' argument from many mm functions*.
Not sure I can do much with that before those patches reach mainline.
Once they do, I'll update my patches.
Please let me know if there's a better way to resolve such conflicts.
> > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > > Cc: James Morris <jmorris@namei.org>
> > > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > > Cc: Kostya Serebryany <kcc@google.com>
> > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Sandeep Patil <sspatil@android.com>
> > > > Cc: Laura Abbott <labbott@redhat.com>
> > > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > Cc: Jann Horn <jannh@google.com>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: linux-mm@kvack.org
> > > > Cc: linux-security-module@vger.kernel.org
> > > > Cc: kernel-hardening@lists.openwall.com
> > > > ---
> > > > include/linux/gfp.h | 6 +++++-
> > > > include/linux/mm.h | 2 +-
> > > > kernel/kexec_core.c | 2 +-
> > > > mm/slab.c | 2 +-
> > > > mm/slob.c | 3 ++-
> > > > mm/slub.c | 1 +
> > > > 6 files changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > > > index fdab7de7490d..66d7f5604fe2 100644
> > > > --- a/include/linux/gfp.h
> > > > +++ b/include/linux/gfp.h
> > > > @@ -44,6 +44,7 @@ struct vm_area_struct;
> > > > #else
> > > > #define ___GFP_NOLOCKDEP 0
> > > > #endif
> > > > +#define ___GFP_NOINIT 0x1000000u
> > >
> > > I mentioned this in the other patch, but I think this needs to be
> > > moved ahead of GFP_NOLOCKDEP and adjust the values for GFP_NOLOCKDEP
> > > and to leave the IS_ENABLED() test in __GFP_BITS_SHIFT alone.
> > Do we really need this blinking GFP_NOLOCKDEP bit at all?
> > This approach doesn't scale, we can't even have a second feature that
> > has a bit depending on the config settings.
> > Cannot we just fix the number of bits instead?
> >
> > > > /* If the above are modified, __GFP_BITS_SHIFT may need updating */
> > > >
> > > > /*
> > > > @@ -208,16 +209,19 @@ struct vm_area_struct;
> > > > * %__GFP_COMP address compound page metadata.
> > > > *
> > > > * %__GFP_ZERO returns a zeroed page on success.
> > > > + *
> > > > + * %__GFP_NOINIT requests non-initialized memory from the underlying allocator.
> > > > */
> > > > #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)
> > > > #define __GFP_COMP ((__force gfp_t)___GFP_COMP)
> > > > #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)
> > > > +#define __GFP_NOINIT ((__force gfp_t)___GFP_NOINIT)
> > > >
> > > > /* Disable lockdep for GFP context tracking */
> > > > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
> > > >
> > > > /* Room for N __GFP_FOO bits */
> > > > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
> > > > +#define __GFP_BITS_SHIFT (25)
> > >
> > > AIUI, this will break non-CONFIG_LOCKDEP kernels: it should just be:
> > >
> > > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
> > > +#define __GFP_BITS_SHIFT (24 + IS_ENABLED(CONFIG_LOCKDEP))
> > >
> > > > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
> > > >
> > > > /**
> > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > index ee1a1092679c..8ab152750eb4 100644
> > > > --- a/include/linux/mm.h
> > > > +++ b/include/linux/mm.h
> > > > @@ -2618,7 +2618,7 @@ DECLARE_STATIC_KEY_FALSE(init_on_alloc);
> > > > static inline bool want_init_on_alloc(gfp_t flags)
> > > > {
> > > > if (static_branch_unlikely(&init_on_alloc))
> > > > - return true;
> > > > + return !(flags & __GFP_NOINIT);
> > > > return flags & __GFP_ZERO;
> > >
> > > What do you think about renaming __GFP_NOINIT to __GFP_NO_AUTOINIT or something?
> > >
> > > Regardless, yes, this is nice.
> > >
> > > --
> > > Kees Cook
> >
> >
> >
> > --
> > Alexander Potapenko
> > Software Engineer
> >
> > Google Germany GmbH
> > Erika-Mann-Straße, 33
> > 80636 München
> >
> > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> > Registergericht und -nummer: Hamburg, HRB 86891
> > Sitz der Gesellschaft: Hamburg
> >
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 3/4] gfp: mm: introduce __GFP_NOINIT
From: Souptick Joarder @ 2019-05-15 10:06 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Andrew Morton, Christoph Lameter, Laura Abbott,
Linux-MM, linux-security-module, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Randy Dunlap,
Jann Horn, Mark Rutland, Matthew Wilcox
In-Reply-To: <CAG_fn=XzWcF=_L1zEU6Gd++u00N=j9GptVLvYOj0_kF0HRu+ig@mail.gmail.com>
On Tue, May 14, 2019 at 8:10 PM Alexander Potapenko <glider@google.com> wrote:
>
> From: Souptick Joarder <jrdr.linux@gmail.com>
> Date: Sat, May 11, 2019 at 9:28 AM
> To: Alexander Potapenko
> Cc: Kees Cook, Andrew Morton, Christoph Lameter, Laura Abbott,
> Linux-MM, linux-security-module, Kernel Hardening, Masahiro Yamada,
> James Morris, Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
> Dmitry Vyukov, Sandeep Patil, Randy Dunlap, Jann Horn, Mark Rutland,
> Matthew Wilcox
>
> > On Thu, May 9, 2019 at 6:53 PM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > From: Kees Cook <keescook@chromium.org>
> > > Date: Wed, May 8, 2019 at 9:16 PM
> > > To: Alexander Potapenko
> > > Cc: Andrew Morton, Christoph Lameter, Kees Cook, Laura Abbott,
> > > Linux-MM, linux-security-module, Kernel Hardening, Masahiro Yamada,
> > > James Morris, Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
> > > Dmitry Vyukov, Sandeep Patil, Randy Dunlap, Jann Horn, Mark Rutland
> > >
> > > > On Wed, May 8, 2019 at 8:38 AM Alexander Potapenko <glider@google.com> wrote:
> > > > > When passed to an allocator (either pagealloc or SL[AOU]B), __GFP_NOINIT
> > > > > tells it to not initialize the requested memory if the init_on_alloc
> > > > > boot option is enabled. This can be useful in the cases newly allocated
> > > > > memory is going to be initialized by the caller right away.
> > > > >
> > > > > __GFP_NOINIT doesn't affect init_on_free behavior, except for SLOB,
> > > > > where init_on_free implies init_on_alloc.
> > > > >
> > > > > __GFP_NOINIT basically defeats the hardening against information leaks
> > > > > provided by init_on_alloc, so one should use it with caution.
> > > > >
> > > > > This patch also adds __GFP_NOINIT to alloc_pages() calls in SL[AOU]B.
> > > > > Doing so is safe, because the heap allocators initialize the pages they
> > > > > receive before passing memory to the callers.
> > > > >
> > > > > Slowdown for the initialization features compared to init_on_free=0,
> > > > > init_on_alloc=0:
> > > > >
> > > > > hackbench, init_on_free=1: +6.84% sys time (st.err 0.74%)
> > > > > hackbench, init_on_alloc=1: +7.25% sys time (st.err 0.72%)
> > > > >
> > > > > Linux build with -j12, init_on_free=1: +8.52% wall time (st.err 0.42%)
> > > > > Linux build with -j12, init_on_free=1: +24.31% sys time (st.err 0.47%)
> > > > > Linux build with -j12, init_on_alloc=1: -0.16% wall time (st.err 0.40%)
> > > > > Linux build with -j12, init_on_alloc=1: +1.24% sys time (st.err 0.39%)
> > > > >
> > > > > The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> > > > > baseline is within the standard error.
> > > > >
> >
> > Not sure, but I think this patch will clash with Matthew's posted patch series
> > *Remove 'order' argument from many mm functions*.
> Not sure I can do much with that before those patches reach mainline.
> Once they do, I'll update my patches.
> Please let me know if there's a better way to resolve such conflicts.
I just thought to highlight about a possible conflict. Nothing else :)
IMO, if other patch series merge into -next tree before this,
then this series can be updated against -next.
... And I am sure others will have a better suggestion.
> > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > > > Cc: James Morris <jmorris@namei.org>
> > > > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > > > Cc: Kostya Serebryany <kcc@google.com>
> > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Cc: Sandeep Patil <sspatil@android.com>
> > > > > Cc: Laura Abbott <labbott@redhat.com>
> > > > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > > Cc: Jann Horn <jannh@google.com>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: linux-mm@kvack.org
> > > > > Cc: linux-security-module@vger.kernel.org
> > > > > Cc: kernel-hardening@lists.openwall.com
> > > > > ---
> > > > > include/linux/gfp.h | 6 +++++-
> > > > > include/linux/mm.h | 2 +-
> > > > > kernel/kexec_core.c | 2 +-
> > > > > mm/slab.c | 2 +-
> > > > > mm/slob.c | 3 ++-
> > > > > mm/slub.c | 1 +
> > > > > 6 files changed, 11 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > > > > index fdab7de7490d..66d7f5604fe2 100644
> > > > > --- a/include/linux/gfp.h
> > > > > +++ b/include/linux/gfp.h
> > > > > @@ -44,6 +44,7 @@ struct vm_area_struct;
> > > > > #else
> > > > > #define ___GFP_NOLOCKDEP 0
> > > > > #endif
> > > > > +#define ___GFP_NOINIT 0x1000000u
> > > >
> > > > I mentioned this in the other patch, but I think this needs to be
> > > > moved ahead of GFP_NOLOCKDEP and adjust the values for GFP_NOLOCKDEP
> > > > and to leave the IS_ENABLED() test in __GFP_BITS_SHIFT alone.
> > > Do we really need this blinking GFP_NOLOCKDEP bit at all?
> > > This approach doesn't scale, we can't even have a second feature that
> > > has a bit depending on the config settings.
> > > Cannot we just fix the number of bits instead?
> > >
> > > > > /* If the above are modified, __GFP_BITS_SHIFT may need updating */
> > > > >
> > > > > /*
> > > > > @@ -208,16 +209,19 @@ struct vm_area_struct;
> > > > > * %__GFP_COMP address compound page metadata.
> > > > > *
> > > > > * %__GFP_ZERO returns a zeroed page on success.
> > > > > + *
> > > > > + * %__GFP_NOINIT requests non-initialized memory from the underlying allocator.
> > > > > */
> > > > > #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)
> > > > > #define __GFP_COMP ((__force gfp_t)___GFP_COMP)
> > > > > #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)
> > > > > +#define __GFP_NOINIT ((__force gfp_t)___GFP_NOINIT)
> > > > >
> > > > > /* Disable lockdep for GFP context tracking */
> > > > > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
> > > > >
> > > > > /* Room for N __GFP_FOO bits */
> > > > > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
> > > > > +#define __GFP_BITS_SHIFT (25)
> > > >
> > > > AIUI, this will break non-CONFIG_LOCKDEP kernels: it should just be:
> > > >
> > > > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
> > > > +#define __GFP_BITS_SHIFT (24 + IS_ENABLED(CONFIG_LOCKDEP))
> > > >
> > > > > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
> > > > >
> > > > > /**
> > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > index ee1a1092679c..8ab152750eb4 100644
> > > > > --- a/include/linux/mm.h
> > > > > +++ b/include/linux/mm.h
> > > > > @@ -2618,7 +2618,7 @@ DECLARE_STATIC_KEY_FALSE(init_on_alloc);
> > > > > static inline bool want_init_on_alloc(gfp_t flags)
> > > > > {
> > > > > if (static_branch_unlikely(&init_on_alloc))
> > > > > - return true;
> > > > > + return !(flags & __GFP_NOINIT);
> > > > > return flags & __GFP_ZERO;
> > > >
> > > > What do you think about renaming __GFP_NOINIT to __GFP_NO_AUTOINIT or something?
> > > >
> > > > Regardless, yes, this is nice.
> > > >
> > > > --
> > > > Kees Cook
> > >
> > >
> > >
> > > --
> > > Alexander Potapenko
> > > Software Engineer
> > >
> > > Google Germany GmbH
> > > Erika-Mann-Straße, 33
> > > 80636 München
> > >
> > > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> > > Registergericht und -nummer: Hamburg, HRB 86891
> > > Sitz der Gesellschaft: Hamburg
> > >
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-15 18:27 UTC (permalink / raw)
To: Sean Christopherson, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux
Cc: Andy Lutomirski, Jarkko Sakkinen, Jethro Beekman, Xing, Cedric,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190515013031.GF1977@linux.intel.com>
Hi, LSM and SELinux people-
We're trying to figure out how SGX fits in with LSMs. For background,
an SGX library is functionally a bit like a DSO, except that it's
nominally resistant to attack from outside and the process of loading
it is complicated. To load an enclave, a program can open
/dev/sgx/enclave, do some ioctls to load the code and data segments
into the enclave, call a special ioctl to "initialize" the enclave,
and then call into the enclave (using special CPU instructions).
One nastiness is that there is not actually a universally agreed upon,
documented file format for enclaves. Windows has an undocumented
format, and there are probably a few others out there. No one really
wants to teach the kernel to parse enclave files.
There are two issues with how this interacts with LSMs:
1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all. The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format. To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation. In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.
2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory. This takes two
forms. First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.
Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable. With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.
With the current version of the SGX driver, to run an enclave, I think
you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
similar, all of which more or less mean that you can run any modified
code you want, and none of which is useful to prevent enclaves from
contain RWX segments.
So my question is: what, if anything, should change to make this work better?
Here's a very vague proposal that's kind of like what I've been
thinking over the past few days. The SGX inode could track, for each
page, a "safe-to-execute" bit. When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute. When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.
The intent is that a page of the enclave is safe-to-execute if that
page was populated from executable memory and not modified since then.
LSMs could then enforce a policy that you can map an enclave page RX
if the page is safe-to-execute, you can map any page you want for
write if there are no executable mappings, and you can only map a page
for write and execute simultaneously if you can EXECMOD permission.
This should allow an enclave to be loaded by userspace from a file
with EXECUTE rights.
So here are my questions:
Are the goals I mentioned reasonable?
Is the design I just outlined reasonable? Would SELinux support this?
Is there a better solution that works well enough?
Thanks, all!
> On May 14, 2019, at 6:30 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
>
>> But thinking this all through, it's a bit more complicated than any of
>> this. Looking at the SELinux code for inspiration, there are quite a
>> few paths, but they boil down to two cases: EXECUTE is the right to
>> map an unmodified file executably, and EXECMOD/EXECMEM (the
>> distinction seems mostly irrelevant) is the right to create (via mmap
>> or mprotect) a modified anonymous file mapping or a non-file-backed
>> mapping that is executable. So, if we do nothing, then mapping an
>> enclave with execute permission will require either EXECUTE on the
>> enclave inode or EXECMOD/EXECMEM, depending on exactly how this gets
>> set up.
>
> If we do literally nothing, then I'm pretty sure mapping an enclave will
> require PROCESS__EXECMEM. The mmap() for the actual enclave is done
> using an anon inode, e.g. from /dev/sgx/enclave. Anon inodes are marked
> private, which means inode_has_perm() will always return "success". The
> only effective check is in file_map_prot_check() when default_noexec is
> true, in which case requesting PROT_EXEC on private inodes requires
> PROCESS__EXECMEM.
>
>> So all is well, sort of. The problem is that I expect there will be
>> people who want enclaves to work in a process that does not have these
>> rights. To make this work, we probably need do some surgery on
>> SELinux. ISTM the act of copying (via the EADD ioctl) data from a
>> PROT_EXEC mapping to an enclave should not be construed as "modifying"
>> the enclave for SELinux purposes. Actually doing this could be
>> awkward, since the same inode will have executable parts and
>> non-executable parts, and SELinux can't really tell the difference.
>
> Rather the do surgery on SELinux, why not go with Cedric's original
> proposal and propagate the permissions from the source VMA to the EPC
> VMA?
Which EPC VMA? Users can map the enclave fd again after EADD,
resulting in a new VMA. And any realistic enclave will presumably
have RO, RW, and RX pages.
> The enclave mmap() from userspace could then be done with RO
> permissions so as to not run afoul of LSMs. Adding PROT_EXEC after
> EADD would require PROCESS__EXECMEM, but that's in line with mprotect()
> on regular memory.
How does this help anything? The driver currently only works with
EXECMEM and, with this change, it still needs EXECMEM.
I think that, if we’re going to make changes along these lines, the
goal should be that you can have an enclave serialized in a file on
disk such that you have EXECUTE on the file, and you should be able to
load and run the enclave without needing EXECMEM. (Unless the enclave
is self-modifying, of course.)
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: James Morris @ 2019-05-15 19:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com>
On Wed, 15 May 2019, Andy Lutomirski wrote:
> There are two issues with how this interacts with LSMs:
>
> 1) LSMs might want to be able to whitelist, blacklist, or otherwise
> restrict what enclaves can run at all. The current proposal that
> everyone seems to dislike the least is to have a .sigstruct file on
> disk that contains a hash and signature of the enclave in a
> CPU-defined format. To initialize an enclave, a program will pass an
> fd to this file, and a new LSM hook can be called to allow or disallow
> the operation. In a SELinux context, the idea is that policy could
> require the .sigstruct file to be labeled with a type like
> sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
> with such a label could run.
The .sigstruct file is for the CPU to consume, not the kernel correct?
How is it bound to the enclave file?
Why not just use an xattr, like security.sgx ?
>
> 2) Just like any other DSO, there are potential issues with how
> enclaves deal with writable vs executable memory. This takes two
> forms. First, a task should probably require EXECMEM, EXECMOD, or
> similar permission to run an enclave that can modify its own text.
> Second, it would be nice if a task that did *not* have EXECMEM,
> EXECMOD, or similar could still run the enclave if it had EXECUTE
> permission on the file containing the enclave.
>
> Currently, this all works because DSOs are run by mmapping the file to
> create multiple VMAs, some of which are executable, non-writable, and
> non-CoWed, and some of which are writable but not executable. With
> SGX, there's only really one inode per enclave (the anon_inode that
> comes form /dev/sgx/enclave), and it can only be sensibly mapped
> MAP_SHARED.
>
> With the current version of the SGX driver, to run an enclave, I think
> you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
> similar, all of which more or less mean that you can run any modified
> code you want, and none of which is useful to prevent enclaves from
> contain RWX segments.
>
> So my question is: what, if anything, should change to make this work better?
Would it be possible to provide multiple fds (perhaps via a pseudo fs
interface) which can be mapped to different types of VMAs?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-15 20:35 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Sean Christopherson, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <alpine.LRH.2.21.1905160543070.19802@namei.org>
On Wed, May 15, 2019 at 12:58 PM James Morris <jmorris@namei.org> wrote:
>
> On Wed, 15 May 2019, Andy Lutomirski wrote:
>
> > There are two issues with how this interacts with LSMs:
> >
> > 1) LSMs might want to be able to whitelist, blacklist, or otherwise
> > restrict what enclaves can run at all. The current proposal that
> > everyone seems to dislike the least is to have a .sigstruct file on
> > disk that contains a hash and signature of the enclave in a
> > CPU-defined format. To initialize an enclave, a program will pass an
> > fd to this file, and a new LSM hook can be called to allow or disallow
> > the operation. In a SELinux context, the idea is that policy could
> > require the .sigstruct file to be labeled with a type like
> > sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
> > with such a label could run.
>
>
> The .sigstruct file is for the CPU to consume, not the kernel correct?
Yes, unless an LSM wants to examine it to make a decision.
>
> How is it bound to the enclave file?
It's not bound to the enclave *file* at all, but it contains a hash
that covers the enclave, so two different files in two different
formats representing exactly the same enclave would get the same hash,
but any change to the enclave would get a different hash.
>
> Why not just use an xattr, like security.sgx ?
Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave? I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want. That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.
>
> >
> > 2) Just like any other DSO, there are potential issues with how
> > enclaves deal with writable vs executable memory. This takes two
> > forms. First, a task should probably require EXECMEM, EXECMOD, or
> > similar permission to run an enclave that can modify its own text.
> > Second, it would be nice if a task that did *not* have EXECMEM,
> > EXECMOD, or similar could still run the enclave if it had EXECUTE
> > permission on the file containing the enclave.
> >
> > Currently, this all works because DSOs are run by mmapping the file to
> > create multiple VMAs, some of which are executable, non-writable, and
> > non-CoWed, and some of which are writable but not executable. With
> > SGX, there's only really one inode per enclave (the anon_inode that
> > comes form /dev/sgx/enclave), and it can only be sensibly mapped
> > MAP_SHARED.
> >
> > With the current version of the SGX driver, to run an enclave, I think
> > you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
> > similar, all of which more or less mean that you can run any modified
> > code you want, and none of which is useful to prevent enclaves from
> > contain RWX segments.
> >
> > So my question is: what, if anything, should change to make this work better?
>
> Would it be possible to provide multiple fds (perhaps via a pseudo fs
> interface) which can be mapped to different types of VMAs?
Maybe. The tricky bit is that, even if there was a separate inode for
the writable and the executable parts of the enclave, I think that
both would have to be mapped MAP_SHARED since MAP_ANONYMOUS is
nonsensical for SGX. This would certainly push more complexity into
the user code. Jarkko?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-15 21:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com>
On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
> 2) Just like any other DSO, there are potential issues with how
> enclaves deal with writable vs executable memory. This takes two
> forms. First, a task should probably require EXECMEM, EXECMOD, or
> similar permission to run an enclave that can modify its own text.
> Second, it would be nice if a task that did *not* have EXECMEM,
> EXECMOD, or similar could still run the enclave if it had EXECUTE
> permission on the file containing the enclave.
>
> Currently, this all works because DSOs are run by mmapping the file to
> create multiple VMAs, some of which are executable, non-writable, and
> non-CoWed, and some of which are writable but not executable. With
> SGX, there's only really one inode per enclave (the anon_inode that
> comes form /dev/sgx/enclave), and it can only be sensibly mapped
> MAP_SHARED.
I was wrong when I said /dev/sgx/enclave creates and returns an anon
inode. I was thinking of the KVM model for creating VMs. SGX creates
an enclave when /dev/sgx/enclave is opened and associates the enclave
with the newly opened /dev/sgx/enclave fd.
Regardless, the fundamental problem remains, mmap() of EPC works on a
single inode.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: James Morris @ 2019-05-15 22:46 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrX_Q6qwNRNF0TL2tgfm1j6DKLX7NVHHmWbMFtk3WnHDKw@mail.gmail.com>
On Wed, 15 May 2019, Andy Lutomirski wrote:
> > Why not just use an xattr, like security.sgx ?
>
> Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
> install an enclave? I think that this decision should be left up the
> administrator, and it should be easy to set up a loose policy where
> anyone can load whatever enclave they want. That's what would happen
> in my proposal if there was no LSM loaded or of the LSM policy didn't
> restrict what .sigstruct files were acceptable.
>
You could try user.sigstruct, which does not require any privs.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-15 23:13 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Sean Christopherson, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <alpine.LRH.2.21.1905160844130.29250@namei.org>
On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
>
> On Wed, 15 May 2019, Andy Lutomirski wrote:
>
> > > Why not just use an xattr, like security.sgx ?
> >
> > Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
> > install an enclave? I think that this decision should be left up the
> > administrator, and it should be easy to set up a loose policy where
> > anyone can load whatever enclave they want. That's what would happen
> > in my proposal if there was no LSM loaded or of the LSM policy didn't
> > restrict what .sigstruct files were acceptable.
> >
>
> You could try user.sigstruct, which does not require any privs.
>
I don't think I understand your proposal. What file would this
attribute be on? What would consume it?
I'm imagining that there's some enclave in a file
crypto_thingy.enclave. There's also a file crypto_thingy.sigstruct.
crypto_thingy.enclave has type lib_t or similar so that it's
executable. crypto_thingy.sigstruct has type sgx_sigstruct_t. The
enclave loader does, in effect:
void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
enclave_offset, len, ...);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
enclave_offset2, len, ...);
etc.
/* Here's where LSMs get to check that the sigstruct is acceptable.
The CPU will check that the sigstruct matches the enclave. */
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);
/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox