| Message ID | 20191121171444.2797-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v0] IMA: Check IMA policy flag | expand |
On 11/21/19 9:14 AM, Lakshmi Ramasubramanian wrote: Hi Mimi, > process_buffer_measurement() needs to check if ima_policy_flag > is set to measure and\or appraise. Not doing this check can > result in kernel panic (such as when process_buffer_measurement() > is called before IMA is initialized). > > This change adds the check in process_buffer_measurement() > to return immediately if ima_policy_flag is set to 0. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > security/integrity/ima/ima_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 60027c643ecd..c9374430bb72 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size, > int pcr = CONFIG_IMA_MEASURE_PCR_IDX; > int action = 0; > > + if (!ima_policy_flag) > + return; > + Please let me know if the above change would be accepted as a standalone patch (like the one in this patch), or, I should include this change as one of the patches in the "Key Measurement" patch set? thanks, -lakshmi
On Mon, 2019-11-25 at 10:23 -0800, Lakshmi Ramasubramanian wrote: > On 11/21/19 9:14 AM, Lakshmi Ramasubramanian wrote: > > Hi Mimi, > > > process_buffer_measurement() needs to check if ima_policy_flag > > is set to measure and\or appraise. Not doing this check can > > result in kernel panic (such as when process_buffer_measurement() > > is called before IMA is initialized). > > > > This change adds the check in process_buffer_measurement() > > to return immediately if ima_policy_flag is set to 0. > > > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > --- > > security/integrity/ima/ima_main.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > index 60027c643ecd..c9374430bb72 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size, > > int pcr = CONFIG_IMA_MEASURE_PCR_IDX; > > int action = 0; > > > > + if (!ima_policy_flag) > > + return; > > + > > Please let me know if the above change would be accepted as a standalone > patch (like the one in this patch), > or, I should include this change as one of the patches in the "Key > Measurement" patch set? As I'm not planning on sending a pull request this open window, so that it doesn't get lost/forgotten, please include it as the first patch in this patch set. Mimi
On 11/25/19 10:30 AM, Mimi Zohar wrote: >> >> Please let me know if the above change would be accepted as a standalone >> patch (like the one in this patch), >> or, I should include this change as one of the patches in the "Key >> Measurement" patch set? > > As I'm not planning on sending a pull request this open window, so > that it doesn't get lost/forgotten, please include it as the first > patch in this patch set. > > Mimi > I have included the change to check ima_policy_flag in process_buffer_measurement() as the 1st patch (PATCH v9 1/6) in the updated patch set I posted today. thanks, -lakshmi
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 60027c643ecd..c9374430bb72 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -651,6 +651,9 @@ static void process_buffer_measurement(const void *buf, int size, int pcr = CONFIG_IMA_MEASURE_PCR_IDX; int action = 0; + if (!ima_policy_flag) + return; + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr, &template_desc); if (!(action & IMA_MEASURE))
process_buffer_measurement() needs to check if ima_policy_flag is set to measure and\or appraise. Not doing this check can result in kernel panic (such as when process_buffer_measurement() is called before IMA is initialized). Please see one instance reported by Eric Snowberg (eric.snowberg@oracle.com) when testing key measurement: https://lore.kernel.org/linux-integrity/20191118223818.3353-1-nramas@linux.microsoft.com/T/#mc243b065659d43a7f5070a2383979542f10d58d1 This change adds the check in process_buffer_measurement() to return immediately if ima_policy_flag is set to 0. [ 1.185105] Loading compiled-in X.509 certificates [ 1.190240] BUG: kernel NULL pointer dereference, address: 0000000000000000 [ 1.191835] #PF: supervisor read access in kernel mode [ 1.193041] #PF: error_code(0x0000) - not-present page [ 1.222749] Call Trace: [ 1.223344] ? crypto_destroy_tfm+0x5f/0xb0 [ 1.224315] ima_get_action+0x2c/0x30 [ 1.225148] process_buffer_measurement+0x1da/0x230 [ 1.226306] ima_post_key_create_or_update+0x3b/0x40 [ 1.227459] key_create_or_update+0x371/0x5c0 [ 1.228494] load_system_certificate_list+0x99/0xfa [ 1.229588] ? system_trusted_keyring_init+0xfb/0xfb [ 1.230705] ? do_early_param+0x95/0x95 [ 1.231574] do_one_initcall+0x4a/0x1fa [ 1.232444] ? do_early_param+0x95/0x95 [ 1.233313] kernel_init_freeable+0x1c2/0x267 [ 1.234300] ? rest_init+0xb0/0xb0 [ 1.235075] kernel_init+0xe/0x110 [ 1.235842] ret_from_fork+0x24/0x50 Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- security/integrity/ima/ima_main.c | 3 +++ 1 file changed, 3 insertions(+)