* [PATCH V9 3/3] KEXEC:Call ima_kexec_cmdline to measure the boot command line args
From: Prakhar Srivastava @ 2019-06-17 18:37 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
In-Reply-To: <20190617183738.14484-1-prsriva02@gmail.com>
During soft reboot(kexec_file_load) boot command line
arguments are not measured.
Call ima hook ima_kexec_cmdline to measure the boot command line
arguments into IMA measurement list.
- call ima_kexec_cmdline from kexec_file_load.
- move the call ima_add_kexec_buffer after the cmdline
args have been measured.
Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
Acked-by: Dave Young <dyoung@redhat.com>
---
kernel/kexec_file.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 072b6ee55e3f..b0c724e5d86c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -198,9 +198,6 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
return ret;
image->kernel_buf_len = size;
- /* IMA needs to pass the measurement list to the next kernel. */
- ima_add_kexec_buffer(image);
-
/* Call arch image probe handlers */
ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
image->kernel_buf_len);
@@ -241,8 +238,14 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
ret = -EINVAL;
goto out;
}
+
+ ima_kexec_cmdline(image->cmdline_buf,
+ image->cmdline_buf_len - 1);
}
+ /* IMA needs to pass the measurement list to the next kernel. */
+ ima_add_kexec_buffer(image);
+
/* Call arch image load handlers */
ldata = arch_kexec_kernel_image_load(image);
--
2.19.1
^ permalink raw reply related
* [PATCH V8 0/3] Add support for measuring the boot command line during kexec_file_load
From: Prakhar Srivastava @ 2019-06-17 18:37 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
The kexec boot command line arguments are not currently being
measured.
Currently during soft reboot(kexec)
- the PCRS are not reset
- the command line arguments used for the next kernel are not measured.
This gives the impression to the secure boot attestation that a cold boot took
place.
For secure boot attestation, it is necessary to measure the kernel
command line. For cold boot, the boot loader can be enhanced to measure
these parameters.
(https://mjg59.dreamwidth.org/48897.html)
This patch set aims to address measuring the boot command line during
soft reboot(kexec_file_load).
To achive the above the patch series does the following
-Add a new ima hook: ima_kexec_cmdline which measures the cmdline args
into the ima log, behind a new ima policy entry KEXEC_CMDLINE.
The kexec cmdline hash is stored in the "d-ng" field of the template data.
-Since the cmldine args cannot be appraised, a new template field(buf) is
added. The template field contains the buffer passed(cmldine args), which
can be used to appraise/attest at a later stage.
The kexec cmdline buffer is stored as HEX in the buf field of the event_data.
-Call the ima_kexec_cmdline(...) hook from kexec_file_load call.
The ima logs need to be carried over to the next kernel, which will be followed
up by other patchsets for x86_64 and arm64.
The kexec cmdline hash is stored in the "d-ng" field of the template data.
and can be verified using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
Changelog:
V9(since V8):
- code cleanup
V8(since V7):
- added a new ima template name "ima-buf"
- code cleanup
V7:
- rebased to next-queued-testing
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-queued-testing
V6:
-add a new ima hook and policy to measure the cmdline
args(ima_kexec_cmdline)
-add a new template field buf to contain the buffer measured.
[suggested by Mimi Zohar]
add new fields to ima_event_data to store/read buffer data.
[suggested by Roberto]
-call ima_kexec_cmdline from kexec_file_load path
v5:
-add a new ima hook and policy to measure the cmdline
args(ima_kexec_cmdline)
-add a new template field buf to contain the buffer measured.
[suggested by Mimi Zohar]
-call ima_kexec_cmdline from kexec_file_load path
v4:
- per feedback from LSM community, removed the LSM hook and renamed the
IMA policy to KEXEC_CMDLINE
v3: (rebase changes to next-general)
- Add policy checks for buffer[suggested by Mimi Zohar]
- use the IMA_XATTR to add buffer
- Add kexec_cmdline used for kexec file load
- Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
v2:
- Add policy checks for buffer[suggested by Mimi Zohar]
- Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
- use the IMA_XATTR to add buffer instead of sig template
v1:
-Add kconfigs to control the ima_buffer_check
-measure the cmdline args suffixed with the kernel file name
-add the buffer to the template sig field.
Prakhar Srivastava (3):
Add a new ima hook ima_kexec_cmdline to measure cmdline args
add a new ima template field buf
call ima_kexec_cmdline to measure the cmdline args
Documentation/ABI/testing/ima_policy | 1 +
Documentation/security/IMA-templates.rst | 2 +-
include/linux/ima.h | 2 +
kernel/kexec_file.c | 8 ++-
security/integrity/ima/ima.h | 3 +
security/integrity/ima/ima_api.c | 5 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 80 +++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 9 +++
security/integrity/ima/ima_template.c | 2 +
security/integrity/ima/ima_template_lib.c | 20 ++++++
security/integrity/ima/ima_template_lib.h | 4 ++
12 files changed, 131 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH V8 0/3] Add support for measuring the boot command line during kexec_file_load
From: Prakhar Srivastava @ 2019-06-17 18:35 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
The kexec boot command line arguments are not currently being
measured.
Currently during soft reboot(kexec)
- the PCRS are not reset
- the command line arguments used for the next kernel are not measured.
This gives the impression to the secure boot attestation that a cold boot took
place.
For secure boot attestation, it is necessary to measure the kernel
command line. For cold boot, the boot loader can be enhanced to measure
these parameters.
(https://mjg59.dreamwidth.org/48897.html)
This patch set aims to address measuring the boot command line during
soft reboot(kexec_file_load).
To achive the above the patch series does the following
-Add a new ima hook: ima_kexec_cmdline which measures the cmdline args
into the ima log, behind a new ima policy entry KEXEC_CMDLINE.
The kexec cmdline hash is stored in the "d-ng" field of the template data.
-Since the cmldine args cannot be appraised, a new template field(buf) is
added. The template field contains the buffer passed(cmldine args), which
can be used to appraise/attest at a later stage.
The kexec cmdline buffer is stored as HEX in the buf field of the event_data.
-Call the ima_kexec_cmdline(...) hook from kexec_file_load call.
The ima logs need to be carried over to the next kernel, which will be followed
up by other patchsets for x86_64 and arm64.
The kexec cmdline hash is stored in the "d-ng" field of the template data.
and can be verified using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
Changelog:
V9(since V8):
- code cleanup
V8(since V7):
- added a new ima template name "ima-buf"
- code cleanup
V7:
- rebased to next-queued-testing
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-queued-testing
V6:
-add a new ima hook and policy to measure the cmdline
args(ima_kexec_cmdline)
-add a new template field buf to contain the buffer measured.
[suggested by Mimi Zohar]
add new fields to ima_event_data to store/read buffer data.
[suggested by Roberto]
-call ima_kexec_cmdline from kexec_file_load path
v5:
-add a new ima hook and policy to measure the cmdline
args(ima_kexec_cmdline)
-add a new template field buf to contain the buffer measured.
[suggested by Mimi Zohar]
-call ima_kexec_cmdline from kexec_file_load path
v4:
- per feedback from LSM community, removed the LSM hook and renamed the
IMA policy to KEXEC_CMDLINE
v3: (rebase changes to next-general)
- Add policy checks for buffer[suggested by Mimi Zohar]
- use the IMA_XATTR to add buffer
- Add kexec_cmdline used for kexec file load
- Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
v2:
- Add policy checks for buffer[suggested by Mimi Zohar]
- Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
- use the IMA_XATTR to add buffer instead of sig template
v1:
-Add kconfigs to control the ima_buffer_check
-measure the cmdline args suffixed with the kernel file name
-add the buffer to the template sig field.
Prakhar Srivastava (3):
Add a new ima hook ima_kexec_cmdline to measure cmdline args
add a new ima template field buf
call ima_kexec_cmdline to measure the cmdline args
Documentation/ABI/testing/ima_policy | 1 +
Documentation/security/IMA-templates.rst | 2 +-
include/linux/ima.h | 2 +
kernel/kexec_file.c | 8 ++-
security/integrity/ima/ima.h | 3 +
security/integrity/ima/ima_api.c | 5 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 80 +++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 9 +++
security/integrity/ima/ima_template.c | 2 +
security/integrity/ima/ima_template_lib.c | 20 ++++++
security/integrity/ima/ima_template_lib.h | 4 ++
12 files changed, 131 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 3/3] KEXEC:Call ima_kexec_cmdline to measure the boot command line args
From: Prakhar Srivastava @ 2019-06-17 18:35 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
In-Reply-To: <20190617183507.14160-1-prsriva02@gmail.com>
During soft reboot(kexec_file_load) boot command line
arguments are not measured.
Call ima hook ima_kexec_cmdline to measure the boot command line
arguments into IMA measurement list.
- call ima_kexec_cmdline from kexec_file_load.
- move the call ima_add_kexec_buffer after the cmdline
args have been measured.
Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
Acked-by: Dave Young <dyoung@redhat.com>
---
kernel/kexec_file.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 072b6ee55e3f..b0c724e5d86c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -198,9 +198,6 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
return ret;
image->kernel_buf_len = size;
- /* IMA needs to pass the measurement list to the next kernel. */
- ima_add_kexec_buffer(image);
-
/* Call arch image probe handlers */
ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
image->kernel_buf_len);
@@ -241,8 +238,14 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
ret = -EINVAL;
goto out;
}
+
+ ima_kexec_cmdline(image->cmdline_buf,
+ image->cmdline_buf_len - 1);
}
+ /* IMA needs to pass the measurement list to the next kernel. */
+ ima_add_kexec_buffer(image);
+
/* Call arch image load handlers */
ldata = arch_kexec_kernel_image_load(image);
--
2.19.1
^ permalink raw reply related
* [PATCH 2/3] IMA:Define a new template field buf
From: Prakhar Srivastava @ 2019-06-17 18:35 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
In-Reply-To: <20190617183507.14160-1-prsriva02@gmail.com>
A buffer(kexec boot command line arguments) measured into IMA
measuremnt list cannot be appraised, without already being
aware of the buffer contents. Since hashes are non-reversible,
raw buffer is needed for validation or regenerating hash for
appraisal/attestation.
Add support to store/read the buffer contents in HEX.
The kexec cmdline hash is stored in the "d-ng" field of the
template data,it can be verified using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
- Add two new fields to ima_event_data to hold the buf and
buf_len [Suggested by Roberto]
- Add a new temaplte field 'buf' to be used to store/read
the buffer data.[Suggested by Mimi]
- Updated process_buffer_meaurement to add the buffer to
ima_event_data. process_buffer_measurement added in
"Define a new IMA hook to measure the boot command line
arguments"
- Add a new template policy name ima-buf to represent
'd-ng|n-ng|buf'
Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
Documentation/security/IMA-templates.rst | 7 ++++---
security/integrity/ima/ima.h | 2 ++
security/integrity/ima/ima_api.c | 4 ++--
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 2 ++
security/integrity/ima/ima_template.c | 3 +++
security/integrity/ima/ima_template_lib.c | 21 +++++++++++++++++++++
security/integrity/ima/ima_template_lib.h | 4 ++++
8 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..fccdbbc984f5 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -69,14 +69,15 @@ descriptors by adding their identifier to the format string
algorithm (field format: [<hash algo>:]digest, where the digest
prefix is shown only if the hash algorithm is not SHA1 or MD5);
- 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature.
-
+ - 'sig': the file signature;
+ - 'buf': the buffer data that was used to generate the hash without size limitations;
Below, there is the list of defined template descriptors:
- "ima": its format is ``d|n``;
- "ima-ng" (default): its format is ``d-ng|n-ng``;
- - "ima-sig": its format is ``d-ng|n-ng|sig``.
+ - "ima-sig": its format is ``d-ng|n-ng|sig``;
+ - "ima-buf": its format is ``d-ng|n-ng|buf``;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a4ad1270bffa..16110180545c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -65,6 +65,8 @@ struct ima_event_data {
struct evm_ima_xattr_data *xattr_value;
int xattr_len;
const char *violation;
+ const void *buf;
+ int buf_len;
};
/* IMA template field data definition */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index ea7d8cbf712f..83ca99d65e4b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -140,7 +140,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
struct ima_template_entry *entry;
struct inode *inode = file_inode(file);
struct ima_event_data event_data = {iint, file, filename, NULL, 0,
- cause};
+ cause, NULL, 0};
int violation = 1;
int result;
@@ -296,7 +296,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
struct inode *inode = file_inode(file);
struct ima_template_entry *entry;
struct ima_event_data event_data = {iint, file, filename, xattr_value,
- xattr_len, NULL};
+ xattr_len, NULL, NULL, 0};
int violation = 0;
if (iint->measured_pcrs & (0x1 << pcr))
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 993d0f1915ff..c8591406c0e2 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
struct ima_template_entry *entry;
struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
- NULL, 0, NULL};
+ NULL, 0, NULL, NULL, 0};
int result = -ENOMEM;
int violation = 0;
struct {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1e233417a7af..84b321ac1ad3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -638,6 +638,8 @@ static void process_buffer_measurement(const void *buf, int size,
goto out;
event_data.filename = eventname;
+ event_data.buf = buf;
+ event_data.buf_len = size;
iint.ima_hash = &hash.hdr;
iint.ima_hash->algo = ima_hash_algo;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index e6e892f31cbd..632f314c0e5a 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
{.name = "ima-ng", .fmt = "d-ng|n-ng"},
{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
+ {.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
{.name = "", .fmt = ""}, /* placeholder for a custom format */
};
@@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
.field_show = ima_show_template_string},
{.field_id = "sig", .field_init = ima_eventsig_init,
.field_show = ima_show_template_sig},
+ {.field_id = "buf", .field_init = ima_eventbuf_init,
+ .field_show = ima_show_template_buf},
};
#define MAX_TEMPLATE_NAME_LEN 15
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 513b457ae900..baf4de45c5aa 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -162,6 +162,12 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
}
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+ struct ima_field_data *field_data)
+{
+ ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
+}
+
/**
* ima_parse_buf() - Parses lengths and data from an input buffer
* @bufstartp: Buffer start address.
@@ -389,3 +395,18 @@ int ima_eventsig_init(struct ima_event_data *event_data,
return ima_write_template_field_data(xattr_value, event_data->xattr_len,
DATA_FMT_HEX, field_data);
}
+
+/*
+ * ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the
+ * template data.
+ */
+int ima_eventbuf_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data)
+{
+ if ((!event_data->buf) || (event_data->buf_len == 0))
+ return 0;
+
+ return ima_write_template_field_data(event_data->buf,
+ event_data->buf_len, DATA_FMT_HEX,
+ field_data);
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6a3d8b831deb..12f1a8578b31 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show,
struct ima_field_data *field_data);
void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
struct ima_field_data *field_data);
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+ struct ima_field_data *field_data);
int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
int maxfields, struct ima_field_data *fields, int *curfields,
unsigned long *len_mask, int enforce_mask, char *bufname);
@@ -42,4 +44,6 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
int ima_eventsig_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
+int ima_eventbuf_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data);
#endif /* __LINUX_IMA_TEMPLATE_LIB_H */
--
2.19.1
^ permalink raw reply related
* [PATCH 1/3] IMA:Define a new hook to measure the kexec boot command line arguments
From: Prakhar Srivastava @ 2019-06-17 18:35 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: zohar, roberto.sassu, Prakhar Srivastava
In-Reply-To: <20190617183507.14160-1-prsriva02@gmail.com>
Currently during soft reboot(kexec_file_load) boot command line
arguments are not measured. Define hooks needed to measure kexec
command line arguments during soft reboot(kexec_file_load).
- A new ima hook ima_kexec_cmdline is defined to be called by the
kexec code.
- A new function process_buffer_measurement is defined to measure
the buffer hash into the IMA measurement list.
- A new func policy KEXEC_CMDLINE is defined to control the
measurement.[Suggested by Mimi]
Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
---
Documentation/ABI/testing/ima_policy | 1 +
include/linux/ima.h | 2 +
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 1 +
security/integrity/ima/ima_main.c | 74 ++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 7 +++
6 files changed, 86 insertions(+)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index b383c1763610..fc376a323908 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -28,6 +28,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
+ [KEXEC_CMDLINE]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index fd9f7cf4cdf5..b42f5a006042 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,7 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
+extern void ima_kexec_cmdline(const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -92,6 +93,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
return;
}
+static inline void ima_kexec_cmdline(const void *buf, int size) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 18b48a6d0b80..a4ad1270bffa 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -185,6 +185,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
hook(KEXEC_KERNEL_CHECK) \
hook(KEXEC_INITRAMFS_CHECK) \
hook(POLICY_CHECK) \
+ hook(KEXEC_CMDLINE) \
hook(MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 78eb11c7ac07..ea7d8cbf712f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,6 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* subj=, obj=, type=, func=, mask=, fsmagic=
* subj,obj, and type: are LSM specific.
* func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
+ * | KEXEC_CMDLINE
* mask: contains the permission mask
* fsmagic: hex value
*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index af341a80118f..1e233417a7af 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -605,6 +605,80 @@ int ima_load_data(enum kernel_load_data_id id)
return 0;
}
+/*
+ * process_buffer_measurement - Measure the buffer to ima log.
+ * @buf: pointer to the buffer that needs to be added to the log.
+ * @size: size of buffer(in bytes).
+ * @eventname: event name to be used for the buffer entry.
+ * @cred: a pointer to a credentials structure for user validation.
+ * @secid: the secid of the task to be validated.
+ *
+ * Based on policy, the buffer is measured into the ima log.
+ */
+static void process_buffer_measurement(const void *buf, int size,
+ const char *eventname,
+ const struct cred *cred, u32 secid)
+{
+ int ret = 0;
+ struct ima_template_entry *entry = NULL;
+ struct integrity_iint_cache iint = {};
+ struct ima_event_data event_data = {.iint = &iint };
+ struct ima_template_desc *template_desc = NULL;
+ struct {
+ struct ima_digest_data hdr;
+ char digest[IMA_MAX_DIGEST_SIZE];
+ } hash = {};
+ int violation = 0;
+ int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+ int action = 0;
+
+ action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
+ &template_desc);
+ if (!(action & IMA_MEASURE))
+ goto out;
+
+ event_data.filename = eventname;
+
+ iint.ima_hash = &hash.hdr;
+ iint.ima_hash->algo = ima_hash_algo;
+ iint.ima_hash->length = hash_digest_size[ima_hash_algo];
+
+ ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
+ if (ret < 0)
+ goto out;
+
+ ret = ima_alloc_init_template(&event_data, &entry, template_desc);
+ if (ret < 0)
+ goto out;
+
+ if (action & IMA_MEASURE)
+ ret = ima_store_template(entry, violation, NULL, buf, pcr);
+
+ if (ret < 0)
+ ima_free_template_entry(entry);
+
+out:
+ return;
+}
+
+/**
+ * ima_kexec_cmdline - measure kexec cmdline boot args
+ * @buf: pointer to buffer
+ * @size: size of buffer
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_kexec_cmdline(const void *buf, int size)
+{
+ u32 secid;
+
+ if (buf && size != 0) {
+ security_task_getsecid(current, &secid);
+ process_buffer_measurement(buf, size, "kexec-cmdline",
+ current_cred(), secid);
+ }
+}
+
static int __init init_ima(void)
{
int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd9b01881d17..4e8bb7eecd08 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -292,6 +292,11 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
+ if (func == KEXEC_CMDLINE) {
+ if ((rule->flags & IMA_FUNC) && (rule->func == func))
+ return true;
+ return false;
+ }
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
return false;
@@ -880,6 +885,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->func = KEXEC_INITRAMFS_CHECK;
else if (strcmp(args[0].from, "POLICY_CHECK") == 0)
entry->func = POLICY_CHECK;
+ else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
+ entry->func = KEXEC_CMDLINE;
else
result = -EINVAL;
if (!result)
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] integrity: Fix __integrity_init_keyring() section mismatch
From: James Morris @ 2019-06-17 18:11 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Nayna Jain, Mimi Zohar, Serge E . Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20190617074452.12901-1-geert@linux-m68k.org>
On Mon, 17 Jun 2019, Geert Uytterhoeven wrote:
> With gcc-4.6.3:
>
> WARNING: vmlinux.o(.text.unlikely+0x24c64): Section mismatch in reference from the function __integrity_init_keyring() to the function .init.text:set_platform_trusted_keys()
> The function __integrity_init_keyring() references
> the function __init set_platform_trusted_keys().
> This is often because __integrity_init_keyring lacks a __init
> annotation or the annotation of set_platform_trusted_keys is wrong.
>
> Indeed, if the compiler decides not to inline __integrity_init_keyring(),
> a warning is issued.
>
> Fix this by adding the missing __init annotation.
>
> Fixes: 9dc92c45177ab70e ("integrity: Define a trusted platform keyring")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] ima: dynamically allocate shash_desc
From: Mimi Zohar @ 2019-06-17 18:07 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: Jarkko Sakkinen, Stefan Berger, linux-integrity,
linux-security-module, linux-kernel
In-Reply-To: <1560786951.4072.103.camel@linux.ibm.com>
On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote:
> On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote:
> > On 32-bit ARM, we get a warning about excessive stack usage when
> > building with clang.
> >
> > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size
> > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,-
> > Wframe-larger-than=]
>
> I'm definitely not seeing this. Is this problem a result of non
> upstreamed patches? For sha1, currently the only possible hash
> algorithm, I'm seeing 664.
Every time a measurement is added to the measurement list, the memory
would be allocated/freed. The frequency of new measurements is policy
dependent. For performance reasons, I'd prefer if the allocation
remains on the stack.
Mimi
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Andy Lutomirski @ 2019-06-17 17:08 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Cedric Xing, LSM List, selinux,
LKML, linux-sgx, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Paul Moore, Eric Paris, Jethro Beekman, Dave Hansen,
Thomas Gleixner, Linus Torvalds, Andrew Morton, nhorman,
pmccallum, Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao,
Andy Shevchenko, Svahn, Kai, Borislav Petkov, Josh Triplett,
Huang, Kai, David Rientjes, Roberts, William C, Philip Tricca
In-Reply-To: <20190617164915.GA25085@linux.intel.com>
On Mon, Jun 17, 2019 at 9:49 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Sun, Jun 16, 2019 at 03:14:51PM -0700, Andy Lutomirski wrote:
> > On Fri, Jun 14, 2019 at 8:38 AM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > > > Andy and/or Cedric, can you please weigh in with a concrete (and practical)
> > > > use case that will break if we go with #1? The auditing issues for #2/#3
> > > > are complex to say the least...
> >
> > The most significant issue I see is the following. Consider two
> > cases. First, an SGX2 enclave that dynamically allocates memory but
> > doesn't execute code from dynamic memory. Second, an SGX2 enclave
> > that *does* execute code from dynamic memory. In #1, the untrusted
> > stack needs to decide whether to ALLOW_EXEC when the memory is
> > allocated, which means that it either needs to assume the worst or it
> > needs to know at allocation time whether the enclave ever intends to
> > change the permission to X.
>
> I'm just not convinced that folks running enclaves that can't communicate
> their basic functionality will care one whit about SELinux restrictions,
> i.e. will happily give EXECMOD even if it's not strictly necessary.
At least when permissions are learned, if there's no ALLOW_EXEC for
EAUG, then EXECMOD won't get learned if there's no eventual attempt to
execute the memory.
>
> > I suppose there's a middle ground. The driver could use model #1 for
> > driver-filled pages and model #2 for dynamic pages. I haven't tried
> > to fully work it out, but I think there would be the ALLOW_READ /
> > ALLOW_WRITE / ALLOW_EXEC flag for EADD-ed pages but, for EAUG-ed
> > pages, there would be a different policy. This might be as simple as
> > internally having four flags instead of three:
> >
> > ALLOW_READ, ALLOW_WRITE, ALLOW_EXEC: as before
> >
> > ALLOW_EXEC_COND: set implicitly by the driver for EAUG.
> >
> > As in #1, if you try to mmap or protect a page with neither ALLOW_EXEC
> > variant, it fails (-EACCES, perhaps). But, if you try to mmap or
> > mprotect an ALLOW_EXEC_COND page with PROT_EXEC, you ask LSM for
> > permission. There is no fancy DIRTY tracking here, since it's
> > reasonable to just act as though *every* ALLOW_EXEC_COND page is
> > dirty. There is no real auditing issue here, since LSM can just log
> > what permission is missing.
> >
> > Does this seem sensible? It might give us the best of #1 and #2.
>
> It would work and is easy to implement *if* SELinux ties permissions to
> the process, as the SIGSTRUCT vma/file won't be available at
> EAUG+mprotect(). I already have a set of patches to that effect, I'll
> send 'em out in a bit.
I'm okay with that.
>
> FWIW, we still need to differentiate W->X from WX on SGX1, i.e. declaring
> ALLOW_WRITE + ALLOW_EXEC shouldn't imply WX. This is also addressed in
> the forthcoming updated RFC.
Sounds good.
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-06-17 16:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Cedric Xing, LSM List, selinux, LKML, linux-sgx,
Jarkko Sakkinen, James Morris, Serge E. Hallyn, Paul Moore,
Eric Paris, Jethro Beekman, Dave Hansen, Thomas Gleixner,
Linus Torvalds, Andrew Morton, nhorman, pmccallum, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes,
Roberts, William C, Philip Tricca
In-Reply-To: <CALCETrXcOQkvMHdh5DgdQ6JAgzsZCNFVEtnQz-5RbNr4vsadDQ@mail.gmail.com>
On Sun, Jun 16, 2019 at 03:14:51PM -0700, Andy Lutomirski wrote:
> On Fri, Jun 14, 2019 at 8:38 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > > Andy and/or Cedric, can you please weigh in with a concrete (and practical)
> > > use case that will break if we go with #1? The auditing issues for #2/#3
> > > are complex to say the least...
>
> The most significant issue I see is the following. Consider two
> cases. First, an SGX2 enclave that dynamically allocates memory but
> doesn't execute code from dynamic memory. Second, an SGX2 enclave
> that *does* execute code from dynamic memory. In #1, the untrusted
> stack needs to decide whether to ALLOW_EXEC when the memory is
> allocated, which means that it either needs to assume the worst or it
> needs to know at allocation time whether the enclave ever intends to
> change the permission to X.
I'm just not convinced that folks running enclaves that can't communicate
their basic functionality will care one whit about SELinux restrictions,
i.e. will happily give EXECMOD even if it's not strictly necessary.
> I suppose there's a middle ground. The driver could use model #1 for
> driver-filled pages and model #2 for dynamic pages. I haven't tried
> to fully work it out, but I think there would be the ALLOW_READ /
> ALLOW_WRITE / ALLOW_EXEC flag for EADD-ed pages but, for EAUG-ed
> pages, there would be a different policy. This might be as simple as
> internally having four flags instead of three:
>
> ALLOW_READ, ALLOW_WRITE, ALLOW_EXEC: as before
>
> ALLOW_EXEC_COND: set implicitly by the driver for EAUG.
>
> As in #1, if you try to mmap or protect a page with neither ALLOW_EXEC
> variant, it fails (-EACCES, perhaps). But, if you try to mmap or
> mprotect an ALLOW_EXEC_COND page with PROT_EXEC, you ask LSM for
> permission. There is no fancy DIRTY tracking here, since it's
> reasonable to just act as though *every* ALLOW_EXEC_COND page is
> dirty. There is no real auditing issue here, since LSM can just log
> what permission is missing.
>
> Does this seem sensible? It might give us the best of #1 and #2.
It would work and is easy to implement *if* SELinux ties permissions to
the process, as the SIGSTRUCT vma/file won't be available at
EAUG+mprotect(). I already have a set of patches to that effect, I'll
send 'em out in a bit.
FWIW, we still need to differentiate W->X from WX on SGX1, i.e. declaring
ALLOW_WRITE + ALLOW_EXEC shouldn't imply WX. This is also addressed in
the forthcoming updated RFC.
> > Follow-up question, is #1 any more palatable if SELinux adds SGX specific
> > permissions and ties them to the process (instead of the vma or sigstruct)?
>
> I'm not sure this makes a difference. It simplifies SIGSTRUCT
> handling, which is handy.
^ permalink raw reply
* Re: [RFC PATCH v2 5/5] security/selinux: Add enclave_load() implementation
From: Jarkko Sakkinen @ 2019-06-17 16:38 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-6-sean.j.christopherson@intel.com>
On Wed, Jun 05, 2019 at 07:11:45PM -0700, Sean Christopherson wrote:
> The goal of selinux_enclave_load() is to provide a facsimile of the
> existing selinux_file_mprotect() and file_map_prot_check() policies,
> but tailored to the unique properties of SGX.
>
> For example, an enclave page is technically backed by a MAP_SHARED file,
> but the "file" is essentially shared memory that is never persisted
> anywhere and also requires execute permissions (for some pages).
>
> The basic concept is to require appropriate execute permissions on the
> source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> an enclave page is being loaded from a regular file, require
> FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> anonymous/private mapping, require PROCESS__EXECMEM since the process
> is essentially executing from the mapping, albeit in a roundabout way.
>
> Note, FILE__READ and FILE__WRITE are intentionally not required even if
> the source page is backed by a regular file. Writes to the enclave page
> are contained to the EPC, i.e. never hit the original file, and read
> permissions have already been vetted (or the VMA doesn't have PROT_READ,
> in which case loading the page into the enclave will fail).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
In the end of the day, the main problem with this patch is that the
existing LSM hooks are generic. I don't we can have specific hooks
for proprietary hardware.
/Jarkko
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Peter Zijlstra @ 2019-06-17 16:24 UTC (permalink / raw)
To: David Howells
Cc: Jann Horn, Greg KH, Al Viro, raven, linux-fsdevel, Linux API,
linux-block, keyrings, linux-security-module, kernel list,
Kees Cook, Kernel Hardening
In-Reply-To: <15401.1559322762@warthog.procyon.org.uk>
On Fri, May 31, 2019 at 06:12:42PM +0100, David Howells wrote:
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > > (and it has already been established that refcount_t doesn't work for
> > > > usage count scenarios)
> > >
> > > ?
> > >
> > > Does that mean struct kref doesn't either?
> >
> > Indeed, since kref is just a pointless wrapper around refcount_t it does
> > not either.
> >
> > The main distinction between a reference count and a usage count is that
> > 0 means different things. For a refcount 0 means dead. For a usage count
> > 0 is merely unused but valid.
>
> Ah - I consider the terms interchangeable.
>
> Take Documentation/filesystems/vfs.txt for instance:
>
> dget: open a new handle for an existing dentry (this just increments
> the usage count)
>
> dput: close a handle for a dentry (decrements the usage count). ...
>
> ...
>
> d_lookup: look up a dentry given its parent and path name component
> It looks up the child of that given name from the dcache
> hash table. If it is found, the reference count is incremented
> and the dentry is returned. The caller must use dput()
> to free the dentry when it finishes using it.
>
> Here we interchange the terms.
>
> Or https://www.kernel.org/doc/gorman/html/understand/understand013.html
> which seems to interchange the terms in reference to struct page.
Right, but we have two distinct set of semantics, I figured it makes
sense to have two different names for them. Do you have an alternative
naming scheme we could use?
Or should we better document our distinction between reference and usage
count?
^ permalink raw reply
* Re: [PATCH] ima: dynamically allocate shash_desc
From: Mimi Zohar @ 2019-06-17 15:55 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: Jarkko Sakkinen, Stefan Berger, linux-integrity,
linux-security-module, linux-kernel
In-Reply-To: <20190617115838.2397872-1-arnd@arndb.de>
On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote:
> On 32-bit ARM, we get a warning about excessive stack usage when
> building with clang.
>
> security/integrity/ima/ima_crypto.c:504:5: error: stack frame size
> of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,-
> Wframe-larger-than=]
I'm definitely not seeing this. Is this problem a result of non
upstreamed patches? For sha1, currently the only possible hash
algorithm, I'm seeing 664.
Mimi
>
> Using kmalloc to get the descriptor reduces this to 320 bytes.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> security/integrity/ima/ima_crypto.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index d4c7b8e1b083..8a66bab4c435 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -461,16 +461,21 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
> struct ima_digest_data *hash,
> struct crypto_shash *tfm)
> {
> - SHASH_DESC_ON_STACK(shash, tfm);
> + struct shash_desc *shash;
> int rc, i;
>
> + shash = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm),
> + GFP_KERNEL);
> + if (!shash)
> + return -ENOMEM;
> +
> shash->tfm = tfm;
>
> hash->length = crypto_shash_digestsize(tfm);
>
> rc = crypto_shash_init(shash);
> if (rc != 0)
> - return rc;
> + goto out;
>
> for (i = 0; i < num_fields; i++) {
> u8 buffer[IMA_EVENT_NAME_LEN_MAX + 1] = { 0 };
> @@ -497,7 +502,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>
> if (!rc)
> rc = crypto_shash_final(shash, hash->digest);
> -
> +out:
> + kfree(shash);
> return rc;
> }
>
^ permalink raw reply
* [PATCH v7 2/2] mm: init: report memory auto-initialization features at boot time
From: Alexander Potapenko @ 2019-06-17 15:10 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter
Cc: Alexander Potapenko, Kees Cook, Dmitry Vyukov, James Morris,
Jann Horn, Kostya Serebryany, Laura Abbott, Mark Rutland,
Masahiro Yamada, Matthew Wilcox, Nick Desaulniers, Randy Dunlap,
Sandeep Patil, Serge E. Hallyn, Souptick Joarder, Marco Elver,
Kaiwan N Billimoria, kernel-hardening, linux-mm,
linux-security-module
In-Reply-To: <20190617151050.92663-1-glider@google.com>
Print the currently enabled stack and heap initialization modes.
Stack initialization is enabled by a config flag, while heap
initialization is configured at boot time with defaults being set
in the config. It's more convenient for the user to have all information
about these hardening measures in one place at boot, so the user can
reason about the expected behavior of the running system.
The possible options for stack are:
- "all" for CONFIG_INIT_STACK_ALL;
- "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
- "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
- "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
- "off" otherwise.
Depending on the values of init_on_alloc and init_on_free boottime
options we also report "heap alloc" and "heap free" as "on"/"off".
In the init_on_free mode initializing pages at boot time may take a
while, so print a notice about that as well. This depends on how much
memory is installed, the memory bandwidth, etc.
On a relatively modern x86 system, it takes about 0.75s/GB to wipe all
memory:
[ 0.418722] mem auto-init: stack:byref_all, heap alloc:off, heap free:on
[ 0.419765] mem auto-init: clearing system memory may take some time...
[ 12.376605] Memory: 16408564K/16776672K available (14339K kernel code, 1397K rwdata, 3756K rodata, 1636K init, 11460K bss, 368108K reserved, 0K cma-reserved)
Signed-off-by: Alexander Potapenko <glider@google.com>
Suggested-by: Kees Cook <keescook@chromium.org>
Acked-by: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Marco Elver <elver@google.com>
Cc: Kaiwan N Billimoria <kaiwan@kaiwantech.com>
Cc: kernel-hardening@lists.openwall.com
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
---
v6:
- update patch description, fixed message about clearing memory
v7:
- rebase the patch, add the Acked-by: tag;
- more description updates as suggested by Kees;
- make report_meminit() static.
---
init/main.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/init/main.c b/init/main.c
index 66a196c5e4c3..ff5803b0841c 100644
--- a/init/main.c
+++ b/init/main.c
@@ -520,6 +520,29 @@ static inline void initcall_debug_enable(void)
}
#endif
+/* Report memory auto-initialization states for this boot. */
+static void __init report_meminit(void)
+{
+ const char *stack;
+
+ if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
+ stack = "all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
+ stack = "byref_all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
+ stack = "byref";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
+ stack = "__user";
+ else
+ stack = "off";
+
+ pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
+ stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
+ want_init_on_free() ? "on" : "off");
+ if (want_init_on_free())
+ pr_info("mem auto-init: clearing system memory may take some time...\n");
+}
+
/*
* Set up kernel memory allocators
*/
@@ -530,6 +553,7 @@ static void __init mm_init(void)
* bigger than MAX_ORDER unless SPARSEMEM.
*/
page_ext_init_flatmem();
+ report_meminit();
mem_init();
kmem_cache_init();
pgtable_init();
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-06-17 15:10 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter, Kees Cook
Cc: Alexander Potapenko, Masahiro Yamada, Michal Hocko, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Jann Horn, Mark Rutland, Marco Elver, linux-mm,
linux-security-module, kernel-hardening
In-Reply-To: <20190617151050.92663-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 two exceptions are slab caches with
constructors and SLAB_TYPESAFE_BY_RCU flag. Those are never
zero-initialized to preserve their semantics.
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.
The new features are also going to pave the way for hardware memory
tagging (e.g. arm64's MTE), which will require both on_alloc and on_free
hooks to set the tags for heap objects. With MTE, tagging will have the
same cost as memory initialization.
Although init_on_free is rather costly, there are paranoid use-cases where
in-memory data lifetime is desired to be minimized. There are various
arguments for/against the realism of the associated threat models, but
given that we'll need the infrastructre for MTE anyway, and there are
people who want wipe-on-free behavior no matter what the performance cost,
it seems reasonable to include it in this series.
Signed-off-by: Alexander Potapenko <glider@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
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: Michal Hocko <mhocko@kernel.org>
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: Marco Elver <elver@google.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
v3:
- don't call kernel_init_free_pages() from memblock_free_pages()
- adopted some Kees' comments for the patch description
v4:
- use NULL instead of 0 in slab_alloc_node() (found by kbuild test robot)
- don't write to NULL object in slab_alloc_node() (found by Android
testing)
v5:
- adjusted documentation wording as suggested by Kees
- disable SLAB_POISON if auto-initialization is on
- don't wipe RCU cache allocations made without __GFP_ZERO
- dropped SLOB support
v7:
- rebase the patch, added the Acked-by: tag
---
.../admin-guide/kernel-parameters.txt | 9 +++
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 | 63 ++++++++++++++++---
mm/slab.c | 16 ++++-
mm/slab.h | 19 ++++++
mm/slub.c | 33 ++++++++--
net/core/sock.c | 2 +-
security/Kconfig.hardening | 29 +++++++++
11 files changed, 180 insertions(+), 19 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..84ee1121a2b9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1673,6 +1673,15 @@
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 dd0b5f4e1e45..96be2604f313 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2696,6 +2696,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 8c94c89a6f7e..e164012d3491 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -378,7 +378,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 d66bc8abe0af..50a3b104a491 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -136,6 +136,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
@@ -1090,6 +1132,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)
{
@@ -1142,6 +1192,8 @@ 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);
@@ -2020,8 +2072,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
@@ -2075,13 +2127,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 f7117ad9b3a3..98a89d7c922d 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1830,6 +1830,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;
@@ -3263,7 +3271,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);
@@ -3320,7 +3328,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);
@@ -3441,6 +3449,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);
@@ -3528,7 +3538,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..31032d488b29 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -524,4 +524,23 @@ 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)) {
+ if (c->ctor)
+ return false;
+ if (c->flags & SLAB_TYPESAFE_BY_RCU)
+ return flags & __GFP_ZERO;
+ return true;
+ }
+ 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 || (c->flags & SLAB_TYPESAFE_BY_RCU));
+ return false;
+}
+
#endif /* MM_SLAB_H */
diff --git a/mm/slub.c b/mm/slub.c
index cd04dbd2b5d0..9c4a8b9a955c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1279,6 +1279,12 @@ static int __init setup_slub_debug(char *str)
if (*str == ',')
slub_debug_slabs = str + 1;
out:
+ if ((static_branch_unlikely(&init_on_alloc) ||
+ static_branch_unlikely(&init_on_free)) &&
+ (slub_debug & SLAB_POISON)) {
+ pr_warn("disabling SLAB_POISON: can't be used together with memory auto-initialization\n");
+ slub_debug &= ~SLAB_POISON;
+ }
return 1;
}
@@ -1424,6 +1430,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.
@@ -1433,9 +1452,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;
@@ -2741,8 +2758,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 (unlikely(slab_want_init_on_free(s)) && object)
+ *(void **)object = NULL;
- 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 +3186,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 2b3701958486..8ed13d2487b2 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1596,7 +1596,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 c6cb2d9b2905..a1ffe2eb4d5f 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -160,6 +160,35 @@ 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 "Enable heap memory zeroing on allocation by default"
+ help
+ This has the effect of setting "init_on_alloc=1" on the kernel
+ command line. This can be disabled with "init_on_alloc=0".
+ When "init_on_alloc" is enabled, all page allocator and slab
+ allocator memory will be zeroed when allocated, eliminating
+ many kinds of "uninitialized heap memory" flaws, especially
+ heap content exposures. The performance impact varies by
+ workload, but most cases see <1% impact. Some synthetic
+ workloads have measured as high as 7%.
+
+config INIT_ON_FREE_DEFAULT_ON
+ bool "Enable heap memory zeroing on free by default"
+ help
+ This has the effect of setting "init_on_free=1" on the kernel
+ command line. This can be disabled with "init_on_free=0".
+ Similar to "init_on_alloc", when "init_on_free" is enabled,
+ all page allocator and slab allocator memory will be zeroed
+ when freed, eliminating many kinds of "uninitialized heap memory"
+ flaws, especially heap content exposures. The primary difference
+ with "init_on_free" is that data lifetime in memory is reduced,
+ as anything freed is wiped immediately, making live forensics or
+ cold boot memory attacks unable to recover freed memory contents.
+ The performance impact varies by workload, but is more expensive
+ than "init_on_alloc" due to the negative cache effects of
+ touching "cold" memory areas. Most cases see 3-5% impact. Some
+ synthetic workloads have measured as high as 8%.
+
endmenu
endmenu
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v7 0/3] add init_on_alloc/init_on_free boot options
From: Alexander Potapenko @ 2019-06-17 15:10 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter, Kees Cook
Cc: Alexander Potapenko, Masahiro Yamada, Michal Hocko, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Jann Horn, Mark Rutland, Marco Elver, linux-mm,
linux-security-module, kernel-hardening
Provide init_on_alloc and init_on_free boot options.
These are aimed at preventing possible information leaks and making the
control-flow bugs that depend on uninitialized values more deterministic.
Enabling either of the options guarantees that the memory returned by the
page allocator and SL[AU]B is initialized with zeroes.
SLOB allocator isn't supported at the moment, as its emulation of kmem
caches complicates handling of SLAB_TYPESAFE_BY_RCU caches correctly.
Enabling init_on_free also guarantees that pages and heap objects are
initialized right after they're freed, so it won't be possible to access
stale data by using a dangling pointer.
As suggested by Michal Hocko, right now we don't let the heap users to
disable initialization for certain allocations. There's not enough
evidence that doing so can speed up real-life cases, and introducing
ways to opt-out may result in things going out of control.
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: Michal Hocko <mhocko@kernel.org>
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: Marco Elver <elver@google.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
Alexander Potapenko (2):
mm: security: introduce init_on_alloc=1 and init_on_free=1 boot
options
mm: init: report memory auto-initialization features at boot time
.../admin-guide/kernel-parameters.txt | 9 +++
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 22 +++++++
init/main.c | 24 +++++++
kernel/kexec_core.c | 2 +-
mm/dmapool.c | 2 +-
mm/page_alloc.c | 63 ++++++++++++++++---
mm/slab.c | 16 ++++-
mm/slab.h | 19 ++++++
mm/slub.c | 33 ++++++++--
net/core/sock.c | 2 +-
security/Kconfig.hardening | 29 +++++++++
12 files changed, 204 insertions(+), 19 deletions(-)
---
v3: dropped __GFP_NO_AUTOINIT patches
v5: dropped support for SLOB allocator, handle SLAB_TYPESAFE_BY_RCU
v6: changed wording in boot-time message
v7: dropped the test_meminit.c patch (picked by Andrew Morton already),
minor wording changes
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH] integrity: Fix __integrity_init_keyring() section mismatch
From: Nayna @ 2019-06-17 14:04 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Nayna Jain, Mimi Zohar, James Morris, Serge E . Hallyn,
linux-security-module, linux-kernel
In-Reply-To: <20190617074452.12901-1-geert@linux-m68k.org>
On 06/17/2019 03:44 AM, Geert Uytterhoeven wrote:
> With gcc-4.6.3:
>
> WARNING: vmlinux.o(.text.unlikely+0x24c64): Section mismatch in reference from the function __integrity_init_keyring() to the function .init.text:set_platform_trusted_keys()
> The function __integrity_init_keyring() references
> the function __init set_platform_trusted_keys().
> This is often because __integrity_init_keyring lacks a __init
> annotation or the annotation of set_platform_trusted_keys is wrong.
>
> Indeed, if the compiler decides not to inline __integrity_init_keyring(),
> a warning is issued.
>
> Fix this by adding the missing __init annotation.
>
> Fixes: 9dc92c45177ab70e ("integrity: Define a trusted platform keyring")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Thanks for fixing it.
Reviewed-by: Nayna Jain <nayna@linux.ibm.com>
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [RFC PATCH v4 1/1] Add dm verity root hash pkcs7 sig validation.
From: Milan Broz @ 2019-06-17 13:31 UTC (permalink / raw)
To: Jaskaran Khurana, linux-security-module, linux-kernel,
linux-integrity, linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka
In-Reply-To: <20190613010610.4364-2-jaskarankhurana@linux.microsoft.com>
On 13/06/2019 03:06, Jaskaran Khurana wrote:
...
> Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
> specified for all dm verity volumes and verification must succeed prior
> to creation of device mapper block device.
I had a quick discussion about this and one suggestion was
to add dm-verity kernel module parameter instead of a new config option.
The idea is that if you can control kernel boot commandline, you can add it
there with the same effect (expecting that root device is on dm-verity as well).
Isn't this better option or it is not going to work for you?
Milan
^ permalink raw reply
* [PATCH] ima: dynamically allocate shash_desc
From: Arnd Bergmann @ 2019-06-17 11:20 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: Arnd Bergmann, Jarkko Sakkinen, Stefan Berger, linux-integrity,
linux-security-module, linux-kernel
On 32-bit ARM, we get a warning about excessive stack usage when
building with clang.
security/integrity/ima/ima_crypto.c:504:5: error: stack frame size of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,-Wframe-larger-than=]
Using kmalloc to get the descriptor reduces this to 320 bytes.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
security/integrity/ima/ima_crypto.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index d4c7b8e1b083..8a66bab4c435 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -461,16 +461,21 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
struct ima_digest_data *hash,
struct crypto_shash *tfm)
{
- SHASH_DESC_ON_STACK(shash, tfm);
+ struct shash_desc *shash;
int rc, i;
+ shash = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm),
+ GFP_KERNEL);
+ if (!shash)
+ return -ENOMEM;
+
shash->tfm = tfm;
hash->length = crypto_shash_digestsize(tfm);
rc = crypto_shash_init(shash);
if (rc != 0)
- return rc;
+ goto out;
for (i = 0; i < num_fields; i++) {
u8 buffer[IMA_EVENT_NAME_LEN_MAX + 1] = { 0 };
@@ -497,7 +502,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
if (!rc)
rc = crypto_shash_final(shash, hash->digest);
-
+out:
+ kfree(shash);
return rc;
}
--
2.20.0
^ permalink raw reply related
* [PATCH] integrity: Fix __integrity_init_keyring() section mismatch
From: Geert Uytterhoeven @ 2019-06-17 7:44 UTC (permalink / raw)
To: Nayna Jain, Mimi Zohar, James Morris, Serge E . Hallyn
Cc: linux-security-module, linux-kernel, Geert Uytterhoeven
With gcc-4.6.3:
WARNING: vmlinux.o(.text.unlikely+0x24c64): Section mismatch in reference from the function __integrity_init_keyring() to the function .init.text:set_platform_trusted_keys()
The function __integrity_init_keyring() references
the function __init set_platform_trusted_keys().
This is often because __integrity_init_keyring lacks a __init
annotation or the annotation of set_platform_trusted_keys is wrong.
Indeed, if the compiler decides not to inline __integrity_init_keyring(),
a warning is issued.
Fix this by adding the missing __init annotation.
Fixes: 9dc92c45177ab70e ("integrity: Define a trusted platform keyring")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
security/integrity/digsig.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 4582bc26770a34a7..868ade3e89702ba7 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -69,8 +69,9 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
return -EOPNOTSUPP;
}
-static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
- struct key_restriction *restriction)
+static int __init __integrity_init_keyring(const unsigned int id,
+ key_perm_t perm,
+ struct key_restriction *restriction)
{
const struct cred *cred = current_cred();
int err = 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 00/14] ima: introduce IMA Digest Lists extension
From: Roberto Sassu @ 2019-06-17 6:56 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-doc,
linux-kernel, silviu.vlasceanu
In-Reply-To: <20190614175513.27097-1-roberto.sassu@huawei.com>
On 6/14/2019 7:54 PM, Roberto Sassu wrote:
> This patch set introduces a new IMA extension called IMA Digest Lists.
>
> At early boot, the extension preloads in kernel memory reference digest
> values, that can be compared with actual file digests when files are
> accessed in the system.
>
> The extension will open for new possibilities: PCR with predictable value,
> that can be used for sealing policies associated to data or TPM keys;
> appraisal based on reference digests already provided by Linux distribution
> vendors in the software packages.
>
> The first objective can be achieved because the PCR values does not depend
> on which and when files are measured: the extension measures digest lists
> sequentially and files whose digest is not in the digest list.
>
> The second objective can be reached because the extension is able to
> extract reference measurements from packages (with a user space tool) and
> use it as a source for appraisal verification as the reference came from
> the security.ima xattr. This approach will also reduce the overhead as only
> one signature is verified for many files (as opposed to one signature for
> each file with the current implementation).
>
> This version of the patch set provides a clear separation between current
> and new functionality. First, the new functionality must be explicitly
> enabled from the kernel command line. Second, results of operations
> performed by the extension can be distinguished from those obtained from
> the existing code: measurement entries created by the extension have a
> different PCR; mutable files appraised with the extension have a different
> security.ima type.
>
> The review of this patch set should start from patch 11 and 12, which
> modify the IMA-Measure and IMA-Appraise submodules to use digest lists.
> Patch 1 to 5 are prerequisites. Patch 6 to 10 adds support for digest
> lists. Finally, patch 13 introduces two new policies to measure/appraise
> rootfs and patch 14 adds the documentation (including a flow chart to
> show how IMA has been modified).
>
> The user space tools to configure digest lists are available at:
>
> https://github.com/euleros/digest-list-tools/releases/tag/v0.3
>
> The patch set applies on top of linux-integrity/next-queued-testing
> (73589972b987).
>
> It is necessary to apply also:
> https://patchwork.kernel.org/cover/10957495/
Another dependency is:
https://patchwork.kernel.org/cover/10979341/
Roberto
> To use appraisal, it is necessary to use a modified cpio and a modified
> dracut:
>
> https://github.com/euleros/cpio/tree/xattr-v1
> https://github.com/euleros/dracut/tree/digest-lists
>
> For now, please use it only in a testing environment.
>
>
> Changelog
>
> v3:
> - move ima_lookup_loaded_digest() and ima_add_digest_data_entry() from
> ima_queue.c to ima_digest_list.c
> - remove patch that introduces security.ima_algo
> - add version number and type modifiers to the compact list header
> - remove digest list metadata, all digest lists in the directory are
> accessed
> - move loading of signing keys to user space
> - add violation for both PCRs if they are selected
> - introduce two new appraisal modes
>
> v2:
> - add support for multiple hash algorithms
> - remove RPM parser from the kernel
> - add support for parsing digest lists in user space
>
> v1:
> - add support for immutable/mutable files
> - add support for appraisal with digest lists
>
>
> Roberto Sassu (14):
> ima: read hash algorithm from security.ima even if appraisal is not
> enabled
> ima: generalize ima_read_policy()
> ima: generalize ima_write_policy() and raise uploaded data size limit
> ima: generalize policy file operations
> ima: use ima_show_htable_value to show violations and hash table data
> ima: add parser of compact digest list
> ima: restrict upload of converted digest lists
> ima: prevent usage of digest lists that are not measured/appraised
> ima: introduce new securityfs files
> ima: load parser digests and execute the parser at boot time
> ima: add support for measurement with digest lists
> ima: add support for appraisal with digest lists
> ima: introduce new policies initrd and appraise_initrd
> ima: add Documentation/security/IMA-digest-lists.txt
>
> .../admin-guide/kernel-parameters.txt | 16 +-
> Documentation/security/IMA-digest-lists.txt | 226 +++++++++++++
> include/linux/evm.h | 6 +
> include/linux/fs.h | 1 +
> security/integrity/evm/evm_main.c | 2 +-
> security/integrity/iint.c | 1 +
> security/integrity/ima/Kconfig | 25 ++
> security/integrity/ima/Makefile | 1 +
> security/integrity/ima/ima.h | 32 +-
> security/integrity/ima/ima_api.c | 43 ++-
> security/integrity/ima/ima_appraise.c | 92 +++---
> security/integrity/ima/ima_digest_list.c | 309 ++++++++++++++++++
> security/integrity/ima/ima_digest_list.h | 69 ++++
> security/integrity/ima/ima_fs.c | 224 ++++++++-----
> security/integrity/ima/ima_init.c | 2 +-
> security/integrity/ima/ima_main.c | 81 ++++-
> security/integrity/ima/ima_policy.c | 29 +-
> security/integrity/integrity.h | 22 ++
> 18 files changed, 1018 insertions(+), 163 deletions(-)
> create mode 100644 Documentation/security/IMA-digest-lists.txt
> create mode 100644 security/integrity/ima/ima_digest_list.c
> create mode 100644 security/integrity/ima/ima_digest_list.h
>
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Andy Lutomirski @ 2019-06-16 22:16 UTC (permalink / raw)
To: Xing, Cedric
Cc: Christopherson, Sean J, Stephen Smalley,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
jarkko.sakkinen@linux.intel.com, luto@kernel.org,
jmorris@namei.org, serge@hallyn.com, paul@paul-moore.com,
eparis@parisplace.org, jethro@fortanix.com, Hansen, Dave,
tglx@linutronix.de, torvalds@linux-foundation.org,
akpm@linux-foundation.org, nhorman@redhat.com,
pmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, andriy.shevchenko@linux.intel.com, Svahn, Kai,
bp@alien8.de, josh@joshtriplett.org, Huang, Kai,
rientjes@google.com, Roberts, William C, Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F65504665@ORSMSX116.amr.corp.intel.com>
On Fri, Jun 14, 2019 at 10:16 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Christopherson, Sean J
> > Sent: Thursday, June 13, 2019 5:46 PM
> >
> > On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > > >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> > > >>I haven't looked at this code closely, but it feels like a lot of
> > > >>SGX-specific logic embedded into SELinux that will have to be
> > > >>repeated or reused for every security module. Does SGX not track
> > this state itself?
> > > >
> > > >SGX does track equivalent state.
> > > >
> > > >There are three proposals on the table (I think):
> > > >
> > > > 1. Require userspace to explicitly specificy (maximal) enclave
> > page
> > > > permissions at build time. The enclave page permissions are
> > provided
> > > > to, and checked by, LSMs at enclave build time.
> > > >
> > > > Pros: Low-complexity kernel implementation, straightforward
> > auditing
> > > > Cons: Sullies the SGX UAPI to some extent, may increase
> > complexity of
> > > > SGX2 enclave loaders.
> > > >
> > > > 2. Pre-check LSM permissions and dynamically track mappings to
> > enclave
> > > > pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > > > based on the pre-checked permissions.
> > > >
> > > > Pros: Does not impact SGX UAPI, medium kernel complexity
> > > > Cons: Auditing is complex/weird, requires taking enclave-
> > specific
> > > > lock during mprotect() to query/update tracking.
> > > >
> > > > 3. Implement LSM hooks in SGX to allow LSMs to track enclave
> > regions
> > > > from cradle to grave, but otherwise defer everything to LSMs.
> > > >
> > > > Pros: Does not impact SGX UAPI, maximum flexibility, precise
> > auditing
> > > > Cons: Most complex and "heaviest" kernel implementation of the
> > three,
> > > > pushes more SGX details into LSMs.
> > > >
> > > >My RFC series[1] implements #1. My understanding is that Andy
> > > >(Lutomirski) prefers #2. Cedric's RFC series implements #3.
> > > >
> > > >Perhaps the easiest way to make forward progress is to rule out the
> > > >options we absolutely *don't* want by focusing on the potentially
> > > >blocking issue with each option:
> > > >
> > > > #1 - SGX UAPI funkiness
> > > >
> > > > #2 - Auditing complexity, potential enclave lock contention
> > > >
> > > > #3 - Pushing SGX details into LSMs and complexity of kernel
> > > > implementation
> > > >
> > > >
> > > >[1]
> > > >https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherso
> > > >n@intel.com
> > >
> > > Given the complexity tradeoff, what is the clear motivating example
> > > for why
> > > #1 isn't the obvious choice? That the enclave loader has no way of
> > > knowing a priori whether the enclave will require W->X or WX? But
> > > aren't we better off requiring enclaves to be explicitly marked as
> > > needing such so that we can make a more informed decision about
> > > whether to load them in the first place?
> >
> > Andy and/or Cedric, can you please weigh in with a concrete (and
> > practical) use case that will break if we go with #1? The auditing
> > issues for #2/#3 are complex to say the least...
>
> How does enclave loader provide per-page ALLOW_* flags? And a related question is why they are necessary for enclaves but unnecessary for regular executables or shared objects.
>
> What's the story for SGX2 if mmap()'ing non-existing pages is not allowed?
>
I think it just works. Either you can't mmap() the page until you
have explicitly EAUG-ed it, or you add a new ioctl() that is
effectively "EAUG lazily". The latter would declare that address and
request that it get allocated and EAUGed when faulted, but it wouldn't
actually do the EAUG.
--Andy
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Andy Lutomirski @ 2019-06-16 22:14 UTC (permalink / raw)
To: Sean Christopherson
Cc: Stephen Smalley, Cedric Xing, LSM List, selinux, LKML, linux-sgx,
Jarkko Sakkinen, Andrew Lutomirski, James Morris, Serge E. Hallyn,
Paul Moore, Eric Paris, Jethro Beekman, Dave Hansen,
Thomas Gleixner, Linus Torvalds, Andrew Morton, nhorman,
pmccallum, Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao,
Andy Shevchenko, Svahn, Kai, Borislav Petkov, Josh Triplett,
Huang, Kai, David Rientjes, Roberts, William C, Philip Tricca
In-Reply-To: <20190614153840.GC12191@linux.intel.com>
On Fri, Jun 14, 2019 at 8:38 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 05:46:00PM -0700, Sean Christopherson wrote:
> > On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > > >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> > > >>I haven't looked at this code closely, but it feels like a lot of
> > > >>SGX-specific logic embedded into SELinux that will have to be repeated or
> > > >>reused for every security module. Does SGX not track this state itself?
> > > >
> > > >SGX does track equivalent state.
> > > >
> > > >There are three proposals on the table (I think):
> > > >
> > > > 1. Require userspace to explicitly specificy (maximal) enclave page
> > > > permissions at build time. The enclave page permissions are provided
> > > > to, and checked by, LSMs at enclave build time.
> > > >
> > > > Pros: Low-complexity kernel implementation, straightforward auditing
> > > > Cons: Sullies the SGX UAPI to some extent, may increase complexity of
> > > > SGX2 enclave loaders.
> > > >
> > > > 2. Pre-check LSM permissions and dynamically track mappings to enclave
> > > > pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > > > based on the pre-checked permissions.
> > > >
> > > > Pros: Does not impact SGX UAPI, medium kernel complexity
> > > > Cons: Auditing is complex/weird, requires taking enclave-specific
> > > > lock during mprotect() to query/update tracking.
> > > >
> > > > 3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
> > > > from cradle to grave, but otherwise defer everything to LSMs.
> > > >
> > > > Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
> > > > Cons: Most complex and "heaviest" kernel implementation of the three,
> > > > pushes more SGX details into LSMs.
> > > >
> > > >My RFC series[1] implements #1. My understanding is that Andy (Lutomirski)
> > > >prefers #2. Cedric's RFC series implements #3.
> > > >
> > > >Perhaps the easiest way to make forward progress is to rule out the
> > > >options we absolutely *don't* want by focusing on the potentially blocking
> > > >issue with each option:
> > > >
> > > > #1 - SGX UAPI funkiness
> > > >
> > > > #2 - Auditing complexity, potential enclave lock contention
> > > >
> > > > #3 - Pushing SGX details into LSMs and complexity of kernel implementation
> > > >
> > > >
> > > >[1] https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
> > >
> > > Given the complexity tradeoff, what is the clear motivating example for why
> > > #1 isn't the obvious choice? That the enclave loader has no way of knowing a
> > > priori whether the enclave will require W->X or WX? But aren't we better
> > > off requiring enclaves to be explicitly marked as needing such so that we
> > > can make a more informed decision about whether to load them in the first
> > > place?
> >
> > Andy and/or Cedric, can you please weigh in with a concrete (and practical)
> > use case that will break if we go with #1? The auditing issues for #2/#3
> > are complex to say the least...
The most significant issue I see is the following. Consider two
cases. First, an SGX2 enclave that dynamically allocates memory but
doesn't execute code from dynamic memory. Second, an SGX2 enclave
that *does* execute code from dynamic memory. In #1, the untrusted
stack needs to decide whether to ALLOW_EXEC when the memory is
allocated, which means that it either needs to assume the worst or it
needs to know at allocation time whether the enclave ever intends to
change the permission to X.
I suppose there's a middle ground. The driver could use model #1 for
driver-filled pages and model #2 for dynamic pages. I haven't tried
to fully work it out, but I think there would be the ALLOW_READ /
ALLOW_WRITE / ALLOW_EXEC flag for EADD-ed pages but, for EAUG-ed
pages, there would be a different policy. This might be as simple as
internally having four flags instead of three:
ALLOW_READ, ALLOW_WRITE, ALLOW_EXEC: as before
ALLOW_EXEC_COND: set implicitly by the driver for EAUG.
As in #1, if you try to mmap or protect a page with neither ALLOW_EXEC
variant, it fails (-EACCES, perhaps). But, if you try to mmap or
mprotect an ALLOW_EXEC_COND page with PROT_EXEC, you ask LSM for
permission. There is no fancy DIRTY tracking here, since it's
reasonable to just act as though *every* ALLOW_EXEC_COND page is
dirty. There is no real auditing issue here, since LSM can just log
what permission is missing.
Does this seem sensible? It might give us the best of #1 and #2.
>
> Follow-up question, is #1 any more palatable if SELinux adds SGX specific
> permissions and ties them to the process (instead of the vma or sigstruct)?
I'm not sure this makes a difference. It simplifies SIGSTRUCT
handling, which is handy.
^ permalink raw reply
* Re: [PATCH] tomoyo: Don't check open/getattr permission on sockets.
From: Tetsuo Handa @ 2019-06-16 6:49 UTC (permalink / raw)
To: Al Viro, linux-fsdevel
Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs, takedakn, David S. Miller
In-Reply-To: <1b5722cc-adbc-035d-5ca1-9aa56e70d312@I-love.SAKURA.ne.jp>
Hello, Al.
Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
management.
Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
Do we need to use d_backing_inode() or d_inode() ?
Regards.
On 2019/06/09 15:41, Tetsuo Handa wrote:
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
>
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
>
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
>
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
>
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
> ---
> security/tomoyo/tomoyo.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..9661b86 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
> */
> static int tomoyo_inode_getattr(const struct path *path)
> {
> + /* It is not safe to call tomoyo_get_socket_name(). */
> + if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
> + return 0;
> return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
> }
>
> @@ -316,6 +319,10 @@ static int tomoyo_file_open(struct file *f)
> /* Don't check read permission here if called from do_execve(). */
> if (current->in_execve)
> return 0;
> + /* Sockets can't be opened by open(). */
> + if (f->f_path.dentry->d_inode &&
> + S_ISSOCK(f->f_path.dentry->d_inode->i_mode))
> + return 0;
> return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
> f->f_flags);
> }
>
^ permalink raw reply
* RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: James Morris @ 2019-06-15 3:53 UTC (permalink / raw)
To: Lubashev, Igor
Cc: Serge Hallyn, linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <720751180a9543cfa205cd527248df7c@ustx2ex-dag1mb5.msg.corp.akamai.com>
On Sat, 15 Jun 2019, Lubashev, Igor wrote:
> > On Friday, June 14, 2019, James Morris wrote:
> Unfortunately, perf is using uid==0 and euid==0 as a "capability bits".
>
>
> In tools/perf/util/evsel.c:
> static bool perf_event_can_profile_kernel(void)
> {
> return geteuid() == 0 || perf_event_paranoid() == -1;
> }
>
> In tools/perf/util/symbol.c:
> static bool symbol__read_kptr_restrict(void)
> {
> ...
> value = ((geteuid() != 0) || (getuid() != 0)) ?
> (atoi(line) != 0) :
> (atoi(line) == 2);
> ...
> }
These are bugs. They should be checking for CAP_SYS_ADMIN.
>
> > Have you considered the example security configuration in
> > Documentation/admin-guide/perf-security.rst ?
>
> Unfortunately, this configuration does not work, unless you reset
> /proc/sys/kernel/perf_event_paranoid to a permissive level (see code
> above). We have perf_event_paranoid set to 2. If it worked, we could had
> implemented the same capability-based policy in the wrapper.
This is not necessary for a process which has CAP_SYS_ADMIN.
--
James Morris
<jmorris@namei.org>
^ 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