| Message ID | 20240122183804.3293904-2-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 1/22/24 13:37, Tushar Sugandhi wrote: > Refactor ima_dump_measurement_list() to move the memory allocation part > to a separate function ima_alloc_kexec_file_buf() which allocates buffer > of size 'kexec_segment_size' at kexec 'load'. Make the local variable > ima_kexec_file in function ima_dump_measurement_list() as local static to > the file, so that it can be accessed from ima_alloc_kexec_file_buf(). > Make necessary changes to the function ima_add_kexec_buffer() to call the > above two functions. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- > 1 file changed, 67 insertions(+), 29 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 419dc405c831..99daac355c70 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,62 +15,93 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +static struct seq_file ima_kexec_file; > + > +static void ima_free_kexec_file_buf(struct seq_file *sf) > +{ > + vfree(sf->buf); > + sf->buf = NULL; > + sf->size = 0; > + sf->read_pos = 0; > + sf->count = 0; > +} > + > +static int ima_alloc_kexec_file_buf(size_t segment_size) > +{ > + /* > + * kexec 'load' may be called multiple times. > + * Free and realloc the buffer only if the segment_size is > + * changed from the previous kexec 'load' call. > + */ > + if (ima_kexec_file.buf && > + ima_kexec_file.size == segment_size && > + ima_kexec_file.read_pos == 0 && > + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) > + return 0; > + > + ima_free_kexec_file_buf(&ima_kexec_file); > + > + /* segment size can't change between kexec load and execute */ > + ima_kexec_file.buf = vmalloc(segment_size); > + if (!ima_kexec_file.buf) > + return -ENOMEM; > + > + ima_kexec_file.size = segment_size; > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ > + > + return 0; > +} > + > static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, > unsigned long segment_size) > { > struct ima_queue_entry *qe; > - struct seq_file file; > struct ima_kexec_hdr khdr; > - int ret = 0; > > - /* segment size can't change between kexec load and execute */ > - file.buf = vmalloc(segment_size); > - if (!file.buf) { > - ret = -ENOMEM; > - goto out; > + if (!ima_kexec_file.buf) { > + *buffer_size = 0; > + *buffer = NULL; > + pr_err("%s: Kexec file buf not allocated\n", __func__); > + return -EINVAL; > } > > - file.size = segment_size; > - file.read_pos = 0; > - file.count = sizeof(khdr); /* reserved space */ > - > memset(&khdr, 0, sizeof(khdr)); > khdr.version = 1; > + > + /* Copy as many IMA measurements list records as possible */ > list_for_each_entry_rcu(qe, &ima_measurements, later) { > - if (file.count < file.size) { > + if (ima_kexec_file.count < ima_kexec_file.size) { > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > - ret = -EINVAL; > + pr_err("%s: IMA log file is too big for Kexec buf\n", > + __func__); > break; > } > } > > - if (ret < 0) > - goto out; > - > /* > * fill in reserved space with some buffer details > * (eg. version, buffer size, number of measurements) > */ > - khdr.buffer_size = file.count; > + khdr.buffer_size = ima_kexec_file.count; > if (ima_canonical_fmt) { > khdr.version = cpu_to_le16(khdr.version); > khdr.count = cpu_to_le64(khdr.count); > khdr.buffer_size = cpu_to_le64(khdr.buffer_size); > } > - memcpy(file.buf, &khdr, sizeof(khdr)); > + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); > > print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, > - file.buf, file.count < 100 ? file.count : 100, > + ima_kexec_file.buf, ima_kexec_file.count < 100 ? > + ima_kexec_file.count : 100, > true); > > - *buffer_size = file.count; > - *buffer = file.buf; > -out: > - if (ret == -EINVAL) > - vfree(file.buf); > - return ret; > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > + return 0; > } > > /* > @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image) > return; > } > > - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > - kexec_segment_size); > - if (!kexec_buffer) { > + ret = ima_alloc_kexec_file_buf(kexec_segment_size); > + if (ret < 0) { > pr_err("Not enough memory for the kexec measurement buffer.\n"); > return; > } > > + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > + kexec_segment_size); > + if (ret < 0) { > + pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", > + __func__, ret); > + return; > + } > + > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; A dent with this patch when only applying this patch: Two consecutive kexec loads lead to this here: [ 30.670330] IMA buffer at 0x3fff10000, size = 0xf0000 [ 32.519618] ------------[ cut here ]------------ [ 32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c) [ 32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 vfree+0x254/0x340 [ 32.519786] Modules linked in: bonding tls rfkill sunrpc virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath [ 32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112 [ 32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries [ 32.519973] NIP: c0000000004bd004 LR: c0000000004bd000 CTR: c00000000017ef00 [ 32.519986] REGS: c00000004593b670 TRAP: 0700 Not tainted (6.5.0+) [ 32.519999] MSR: 8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 44424842 XER: 00000000 [ 32.520023] CFAR: c0000000001515b0 IRQMASK: 0 GPR00: c0000000004bd000 c00000004593b910 c000000001e17000 0000000000000038 GPR04: 00000000ffffbfff c00000004593b6e8 c00000004593b6e0 00000003f9580000 GPR08: 0000000000000027 c0000003fb707010 0000000000000001 0000000044424842 GPR12: c00000000017ef00 c00000003fff1300 0000000000000000 0000000000000000 GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR20: 0000000000000000 0000000000000000 0000000000000003 0000000000000004 GPR24: 00007fffeab0f68f 000000000000004c 0000000000000000 c00000002bdce400 GPR28: c000000002bf28f0 0000000000000000 c008000004770000 0000000000000000 [ 32.520180] NIP [c0000000004bd004] vfree+0x254/0x340 [ 32.520212] LR [c0000000004bd000] vfree+0x250/0x340 [ 32.520225] Call Trace: [ 32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 (unreliable) [ 32.520250] [c00000004593b990] [c00000000091d590] ima_add_kexec_buffer+0xe0/0x3c0 [ 32.520296] [c00000004593ba90] [c000000000280968] sys_kexec_file_load+0x148/0x9b0 [ 32.520333] [c00000004593bb70] [c00000000002ea84] system_call_exception+0x174/0x320 [ 32.520372] [c00000004593be50] [c00000000000d6a0] system_call_common+0x160/0x2c4 [ 32.520408] --- interrupt: c00 at 0x7fffa52e7ae4 [ 32.520420] NIP: 00007fffa52e7ae4 LR: 0000000108481d8c CTR: 0000000000000000 [ 32.520452] REGS: c00000004593be80 TRAP: 0c00 Not tainted (6.5.0+) [ 32.520483] MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24424202 XER: 00000000 [ 32.520507] IRQMASK: 0 GPR00: 000000000000017e 00007fffeab09470 00007fffa53f6f00 0000000000000003 GPR04: 0000000000000004 000000000000004c 00007fffeab0f68f 0000000000000000 GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR12: 0000000000000000 00007fffa559b280 0000000000000002 0000000000000001 GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR20: 00007fffa53f0454 00007fffa53f0458 0000000000000000 0000000000000001 GPR24: 0000000000000000 00007fffeab0f64d 0000000000000006 0000000000000000 GPR28: 0000000000000003 00007fffeab09530 00007fffeab09b08 0000000000000007 [ 32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4 [ 32.521192] LR [0000000108481d8c] 0x108481d8c [ 32.521587] --- interrupt: c00 [ 32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 <0fe00000> eba10068 4bffff34 2c080000 [ 32.522823] ---[ end trace 0000000000000000 ]--- [ 32.536347] Removed old IMA buffer reservation. [ 32.536473] IMA buffer at 0x3fff10000, size = 0xf0000 This vfree here probably has to go: ret = kexec_add_buffer(&kbuf); if (ret) { pr_err("Error passing over kexec measurement buffer.\n"); vfree(kexec_buffer); return; }
On 1/23/24 21:54, Stefan Berger wrote: > > > On 1/22/24 13:37, Tushar Sugandhi wrote: >> Refactor ima_dump_measurement_list() to move the memory allocation part >> to a separate function ima_alloc_kexec_file_buf() which allocates buffer >> of size 'kexec_segment_size' at kexec 'load'. Make the local variable >> ima_kexec_file in function ima_dump_measurement_list() as local static to >> the file, so that it can be accessed from ima_alloc_kexec_file_buf(). >> Make necessary changes to the function ima_add_kexec_buffer() to call the >> above two functions. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- >> security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- >> 1 file changed, 67 insertions(+), 29 deletions(-) >> >> diff --git a/security/integrity/ima/ima_kexec.c >> b/security/integrity/ima/ima_kexec.c >> index 419dc405c831..99daac355c70 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -15,62 +15,93 @@ >> #include "ima.h" >> #ifdef CONFIG_IMA_KEXEC >> +static struct seq_file ima_kexec_file; >> + >> +static void ima_free_kexec_file_buf(struct seq_file *sf) >> +{ >> + vfree(sf->buf); >> + sf->buf = NULL; >> + sf->size = 0; >> + sf->read_pos = 0; >> + sf->count = 0; >> +} >> + >> +static int ima_alloc_kexec_file_buf(size_t segment_size) >> +{ >> + /* >> + * kexec 'load' may be called multiple times. >> + * Free and realloc the buffer only if the segment_size is >> + * changed from the previous kexec 'load' call. >> + */ >> + if (ima_kexec_file.buf && >> + ima_kexec_file.size == segment_size && >> + ima_kexec_file.read_pos == 0 && >> + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) >> + return 0; >> + >> + ima_free_kexec_file_buf(&ima_kexec_file); >> + >> + /* segment size can't change between kexec load and execute */ >> + ima_kexec_file.buf = vmalloc(segment_size); >> + if (!ima_kexec_file.buf) >> + return -ENOMEM; >> + >> + ima_kexec_file.size = segment_size; >> + ima_kexec_file.read_pos = 0; >> + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* >> reserved space */ >> + >> + return 0; >> +} >> + >> static int ima_dump_measurement_list(unsigned long *buffer_size, >> void **buffer, >> unsigned long segment_size) >> { >> struct ima_queue_entry *qe; >> - struct seq_file file; >> struct ima_kexec_hdr khdr; >> - int ret = 0; >> - /* segment size can't change between kexec load and execute */ >> - file.buf = vmalloc(segment_size); >> - if (!file.buf) { >> - ret = -ENOMEM; >> - goto out; >> + if (!ima_kexec_file.buf) { >> + *buffer_size = 0; >> + *buffer = NULL; >> + pr_err("%s: Kexec file buf not allocated\n", __func__); >> + return -EINVAL; >> } >> - file.size = segment_size; >> - file.read_pos = 0; >> - file.count = sizeof(khdr); /* reserved space */ >> - >> memset(&khdr, 0, sizeof(khdr)); >> khdr.version = 1; >> + >> + /* Copy as many IMA measurements list records as possible */ >> list_for_each_entry_rcu(qe, &ima_measurements, later) { >> - if (file.count < file.size) { >> + if (ima_kexec_file.count < ima_kexec_file.size) { >> khdr.count++; >> - ima_measurements_show(&file, qe); >> + ima_measurements_show(&ima_kexec_file, qe); >> } else { >> - ret = -EINVAL; >> + pr_err("%s: IMA log file is too big for Kexec buf\n", >> + __func__); >> break; >> } >> } >> - if (ret < 0) >> - goto out; >> - >> /* >> * fill in reserved space with some buffer details >> * (eg. version, buffer size, number of measurements) >> */ >> - khdr.buffer_size = file.count; >> + khdr.buffer_size = ima_kexec_file.count; >> if (ima_canonical_fmt) { >> khdr.version = cpu_to_le16(khdr.version); >> khdr.count = cpu_to_le64(khdr.count); >> khdr.buffer_size = cpu_to_le64(khdr.buffer_size); >> } >> - memcpy(file.buf, &khdr, sizeof(khdr)); >> + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); >> print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, >> - file.buf, file.count < 100 ? file.count : 100, >> + ima_kexec_file.buf, ima_kexec_file.count < 100 ? >> + ima_kexec_file.count : 100, >> true); >> - *buffer_size = file.count; >> - *buffer = file.buf; >> -out: >> - if (ret == -EINVAL) >> - vfree(file.buf); >> - return ret; >> + *buffer_size = ima_kexec_file.count; >> + *buffer = ima_kexec_file.buf; >> + >> + return 0; >> } >> /* >> @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image) >> return; >> } >> - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, >> - kexec_segment_size); >> - if (!kexec_buffer) { >> + ret = ima_alloc_kexec_file_buf(kexec_segment_size); >> + if (ret < 0) { >> pr_err("Not enough memory for the kexec measurement >> buffer.\n"); >> return; >> } >> + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, >> + kexec_segment_size); >> + if (ret < 0) { >> + pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", >> + __func__, ret); >> + return; >> + } >> + >> kbuf.buffer = kexec_buffer; >> kbuf.bufsz = kexec_buffer_size; >> kbuf.memsz = kexec_segment_size; > > > A dent with this patch when only applying this patch: > > Two consecutive kexec loads lead to this here: > > [ 30.670330] IMA buffer at 0x3fff10000, size = 0xf0000 > [ 32.519618] ------------[ cut here ]------------ > [ 32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c) > [ 32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 > vfree+0x254/0x340 > [ 32.519786] Modules linked in: bonding tls rfkill sunrpc > virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs > drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi > scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover > crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks > scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath > [ 32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112 > [ 32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E > (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries > [ 32.519973] NIP: c0000000004bd004 LR: c0000000004bd000 CTR: > c00000000017ef00 > [ 32.519986] REGS: c00000004593b670 TRAP: 0700 Not tainted (6.5.0+) > [ 32.519999] MSR: 8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: > 44424842 XER: 00000000 > [ 32.520023] CFAR: c0000000001515b0 IRQMASK: 0 > GPR00: c0000000004bd000 c00000004593b910 > c000000001e17000 0000000000000038 > GPR04: 00000000ffffbfff c00000004593b6e8 > c00000004593b6e0 00000003f9580000 > GPR08: 0000000000000027 c0000003fb707010 > 0000000000000001 0000000044424842 > GPR12: c00000000017ef00 c00000003fff1300 > 0000000000000000 0000000000000000 > GPR16: 0000000000000000 0000000000000000 > 0000000000000000 0000000000000000 > GPR20: 0000000000000000 0000000000000000 > 0000000000000003 0000000000000004 > GPR24: 00007fffeab0f68f 000000000000004c > 0000000000000000 c00000002bdce400 > GPR28: c000000002bf28f0 0000000000000000 > c008000004770000 0000000000000000 > [ 32.520180] NIP [c0000000004bd004] vfree+0x254/0x340 > [ 32.520212] LR [c0000000004bd000] vfree+0x250/0x340 > [ 32.520225] Call Trace: > [ 32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 > (unreliable) > [ 32.520250] [c00000004593b990] [c00000000091d590] > ima_add_kexec_buffer+0xe0/0x3c0 > [ 32.520296] [c00000004593ba90] [c000000000280968] > sys_kexec_file_load+0x148/0x9b0 > [ 32.520333] [c00000004593bb70] [c00000000002ea84] > system_call_exception+0x174/0x320 > [ 32.520372] [c00000004593be50] [c00000000000d6a0] > system_call_common+0x160/0x2c4 > [ 32.520408] --- interrupt: c00 at 0x7fffa52e7ae4 > [ 32.520420] NIP: 00007fffa52e7ae4 LR: 0000000108481d8c CTR: > 0000000000000000 > [ 32.520452] REGS: c00000004593be80 TRAP: 0c00 Not tainted (6.5.0+) > [ 32.520483] MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> CR: > 24424202 XER: 00000000 > [ 32.520507] IRQMASK: 0 > GPR00: 000000000000017e 00007fffeab09470 > 00007fffa53f6f00 0000000000000003 > GPR04: 0000000000000004 000000000000004c > 00007fffeab0f68f 0000000000000000 > GPR08: 0000000000000000 0000000000000000 > 0000000000000000 0000000000000000 > GPR12: 0000000000000000 00007fffa559b280 > 0000000000000002 0000000000000001 > GPR16: 0000000000000000 0000000000000000 > 0000000000000000 0000000000000000 > GPR20: 00007fffa53f0454 00007fffa53f0458 > 0000000000000000 0000000000000001 > GPR24: 0000000000000000 00007fffeab0f64d > 0000000000000006 0000000000000000 > GPR28: 0000000000000003 00007fffeab09530 > 00007fffeab09b08 0000000000000007 > [ 32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4 > [ 32.521192] LR [0000000108481d8c] 0x108481d8c > [ 32.521587] --- interrupt: c00 > [ 32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 > 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 > <0fe00000> eba10068 4bffff34 2c080000 > [ 32.522823] ---[ end trace 0000000000000000 ]--- > [ 32.536347] Removed old IMA buffer reservation. > [ 32.536473] IMA buffer at 0x3fff10000, size = 0xf0000 > > This vfree here probably has to go: > > ret = kexec_add_buffer(&kbuf); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > vfree(kexec_buffer); > return; > } > The vfree may need to be removed or replaced with ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone. I got rid of this issue later with this: static void ima_reset_kexec_file(struct seq_file *sf) { sf->buf = NULL; sf->size = 0; sf->read_pos = 0; sf->count = 0; } static void ima_free_kexec_file_buf(struct seq_file *sf) { vfree(sf->buf); ima_reset_kexec_file(sf); } [...] @@ -170,6 +175,9 @@ void ima_add_kexec_buffer(struct kimage *image) image->ima_segment_index = image->nr_segments - 1; image->is_ima_segment_index_set = true; + /* kexec owns kexec_buffer since kexec_add_buffer and will vfree() it */ + ima_reset_kexec_file(&ima_kexec_file); + pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n", kbuf.mem); > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec
Hi Tushar, On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote: Missing from this and the other patch descriptions is the problem description. Please refer to the section titled "Describe your changes" in https://docs.kernel.org/process/submitting-patches.html. "Describe your problem. Whether your patch is a one-line bug fix or 5000 lines of a new feature, there must be an underlying problem that motivated you to do this work. Convince the reviewer that there is a problem worth fixing and that it makes sense for them to read past the first paragraph." In this case, "why" you need to refactor ima_dump_measurement_list() is the problem. For example: Carrying the IMA measurement list across kexec requires allocating a buffer and copying the measurement records. Separate allocating the buffer and copying the measurement records into separate functions in order to allocate the buffer at kexec "load" and copy the measurements at kexec "execute". "Once the problem is established, describe what you are actually doing about it in technical detail. It's important to describe the change in plain English for the reviewer to verify that the code is behaving as you intend it to." > Refactor ima_dump_measurement_list() to move the memory allocation part > to a separate function ima_alloc_kexec_file_buf() which allocates buffer > of size 'kexec_segment_size' at kexec 'load'. Make the local variable > ima_kexec_file in function ima_dump_measurement_list() as local static to > the file, so that it can be accessed from ima_alloc_kexec_file_buf(). > Make necessary changes to the function ima_add_kexec_buffer() to call the > above two functions. Please make this into an unordered list.
Thanks Mimi. On 1/24/24 05:33, Mimi Zohar wrote: > Hi Tushar, > > On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote: > > Missing from this and the other patch descriptions is the problem > description. Please refer to the section titled "Describe your changes" in > https://docs.kernel.org/process/submitting-patches.html. > > "Describe your problem. Whether your patch is a one-line bug fix or 5000 lines > of a new feature, there must be an underlying problem that motivated you to do > this work. Convince the reviewer that there is a problem worth fixing and that > it makes sense for them to read past the first paragraph." > > In this case, "why" you need to refactor ima_dump_measurement_list() is the > problem. > Thanks. I will revisit all the patch descriptions in this series to take into account the 'why' specific to that particular patch. > For example: > > Carrying the IMA measurement list across kexec requires allocating a buffer and > copying the measurement records. Separate allocating the buffer and copying the > measurement records into separate functions in order to allocate the buffer at > kexec "load" and copy the measurements at kexec "execute". > Appreciate you giving an example in this case. I will try to follow it in other patches too. > "Once the problem is established, describe what you are actually doing about it > in technical detail. It's important to describe the change in plain English for > the reviewer to verify that the code is behaving as you intend it to." > >> Refactor ima_dump_measurement_list() to move the memory allocation part >> to a separate function ima_alloc_kexec_file_buf() which allocates buffer >> of size 'kexec_segment_size' at kexec 'load'. Make the local variable >> ima_kexec_file in function ima_dump_measurement_list() as local static to >> the file, so that it can be accessed from ima_alloc_kexec_file_buf(). >> Make necessary changes to the function ima_add_kexec_buffer() to call the >> above two functions. > > Please make this into an unordered list. > Will do. Thanks again. ~Tushar
Thanks for catching this Stefan. On 1/23/24 19:38, Stefan Berger wrote: >>> kbuf.buffer = kexec_buffer; >>> kbuf.bufsz = kexec_buffer_size; >>> kbuf.memsz = kexec_segment_size; >> >> >> A dent with this patch when only applying this patch: >> >> Two consecutive kexec loads lead to this here: >> >> [ 30.670330] IMA buffer at 0x3fff10000, size = 0xf0000 >> [ 32.519618] ------------[ cut here ]------------ >> [ 32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c) >> [ 32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 >> vfree+0x254/0x340 >> [ 32.519786] Modules linked in: bonding tls rfkill sunrpc >> virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs >> drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi >> scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover >> crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks >> scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath >> [ 32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112 >> [ 32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E >> (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries >> [ 32.519973] NIP: c0000000004bd004 LR: c0000000004bd000 CTR: >> c00000000017ef00 >> [ 32.519986] REGS: c00000004593b670 TRAP: 0700 Not tainted (6.5.0+) >> [ 32.519999] MSR: 8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: >> 44424842 XER: 00000000 >> [ 32.520023] CFAR: c0000000001515b0 IRQMASK: 0 >> GPR00: c0000000004bd000 c00000004593b910 >> c000000001e17000 0000000000000038 >> GPR04: 00000000ffffbfff c00000004593b6e8 >> c00000004593b6e0 00000003f9580000 >> GPR08: 0000000000000027 c0000003fb707010 >> 0000000000000001 0000000044424842 >> GPR12: c00000000017ef00 c00000003fff1300 >> 0000000000000000 0000000000000000 >> GPR16: 0000000000000000 0000000000000000 >> 0000000000000000 0000000000000000 >> GPR20: 0000000000000000 0000000000000000 >> 0000000000000003 0000000000000004 >> GPR24: 00007fffeab0f68f 000000000000004c >> 0000000000000000 c00000002bdce400 >> GPR28: c000000002bf28f0 0000000000000000 >> c008000004770000 0000000000000000 >> [ 32.520180] NIP [c0000000004bd004] vfree+0x254/0x340 >> [ 32.520212] LR [c0000000004bd000] vfree+0x250/0x340 >> [ 32.520225] Call Trace: >> [ 32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 >> (unreliable) >> [ 32.520250] [c00000004593b990] [c00000000091d590] >> ima_add_kexec_buffer+0xe0/0x3c0 >> [ 32.520296] [c00000004593ba90] [c000000000280968] >> sys_kexec_file_load+0x148/0x9b0 >> [ 32.520333] [c00000004593bb70] [c00000000002ea84] >> system_call_exception+0x174/0x320 >> [ 32.520372] [c00000004593be50] [c00000000000d6a0] >> system_call_common+0x160/0x2c4 >> [ 32.520408] --- interrupt: c00 at 0x7fffa52e7ae4 >> [ 32.520420] NIP: 00007fffa52e7ae4 LR: 0000000108481d8c CTR: >> 0000000000000000 >> [ 32.520452] REGS: c00000004593be80 TRAP: 0c00 Not tainted (6.5.0+) >> [ 32.520483] MSR: 800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE> >> CR: 24424202 XER: 00000000 >> [ 32.520507] IRQMASK: 0 >> GPR00: 000000000000017e 00007fffeab09470 >> 00007fffa53f6f00 0000000000000003 >> GPR04: 0000000000000004 000000000000004c >> 00007fffeab0f68f 0000000000000000 >> GPR08: 0000000000000000 0000000000000000 >> 0000000000000000 0000000000000000 >> GPR12: 0000000000000000 00007fffa559b280 >> 0000000000000002 0000000000000001 >> GPR16: 0000000000000000 0000000000000000 >> 0000000000000000 0000000000000000 >> GPR20: 00007fffa53f0454 00007fffa53f0458 >> 0000000000000000 0000000000000001 >> GPR24: 0000000000000000 00007fffeab0f64d >> 0000000000000006 0000000000000000 >> GPR28: 0000000000000003 00007fffeab09530 >> 00007fffeab09b08 0000000000000007 >> [ 32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4 >> [ 32.521192] LR [0000000108481d8c] 0x108481d8c >> [ 32.521587] --- interrupt: c00 >> [ 32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 >> 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 >> <0fe00000> eba10068 4bffff34 2c080000 >> [ 32.522823] ---[ end trace 0000000000000000 ]--- >> [ 32.536347] Removed old IMA buffer reservation. >> [ 32.536473] IMA buffer at 0x3fff10000, size = 0xf0000 >> >> This vfree here probably has to go: >> >> ret = kexec_add_buffer(&kbuf); >> if (ret) { >> pr_err("Error passing over kexec measurement >> buffer.\n"); >> vfree(kexec_buffer); >> return; >> } >> > > The vfree may need to be removed or replaced with > ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone. > I got rid of this issue later with this: > > static void ima_reset_kexec_file(struct seq_file *sf) > { > sf->buf = NULL; > sf->size = 0; > sf->read_pos = 0; > sf->count = 0; > } > > static void ima_free_kexec_file_buf(struct seq_file *sf) > { > vfree(sf->buf); > ima_reset_kexec_file(sf); > } > > [...] > I was able to repro the issue by calling kexec 'load' multiple times on patch #1. Good catch, thanks. Earlier I was testing the multiple 'load' scenario on the last patch only. Here onward I will call it on each of the patch individually. > @@ -170,6 +175,9 @@ void ima_add_kexec_buffer(struct kimage *image) > image->ima_segment_index = image->nr_segments - 1; > image->is_ima_segment_index_set = true; > > + /* kexec owns kexec_buffer since kexec_add_buffer and will > vfree() it */ > + ima_reset_kexec_file(&ima_kexec_file); > + > pr_debug("kexec measurement buffer for the loaded kernel at > 0x%lx.\n", > kbuf.mem); > > Thanks for the suggested solution. Looks like you applied it on top of patch #3. I applied it on patch #1, and it seems to be working. I will dig deeper to ensure it doesn't cause any memory leaks, and will incorporate it in v5. Thanks again. ~Tushar >> _______________________________________________ >> kexec mailing list >> kexec@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/kexec
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 419dc405c831..99daac355c70 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,62 +15,93 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +static struct seq_file ima_kexec_file; + +static void ima_free_kexec_file_buf(struct seq_file *sf) +{ + vfree(sf->buf); + sf->buf = NULL; + sf->size = 0; + sf->read_pos = 0; + sf->count = 0; +} + +static int ima_alloc_kexec_file_buf(size_t segment_size) +{ + /* + * kexec 'load' may be called multiple times. + * Free and realloc the buffer only if the segment_size is + * changed from the previous kexec 'load' call. + */ + if (ima_kexec_file.buf && + ima_kexec_file.size == segment_size && + ima_kexec_file.read_pos == 0 && + ima_kexec_file.count == sizeof(struct ima_kexec_hdr)) + return 0; + + ima_free_kexec_file_buf(&ima_kexec_file); + + /* segment size can't change between kexec load and execute */ + ima_kexec_file.buf = vmalloc(segment_size); + if (!ima_kexec_file.buf) + return -ENOMEM; + + ima_kexec_file.size = segment_size; + ima_kexec_file.read_pos = 0; + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ + + return 0; +} + static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, unsigned long segment_size) { struct ima_queue_entry *qe; - struct seq_file file; struct ima_kexec_hdr khdr; - int ret = 0; - /* segment size can't change between kexec load and execute */ - file.buf = vmalloc(segment_size); - if (!file.buf) { - ret = -ENOMEM; - goto out; + if (!ima_kexec_file.buf) { + *buffer_size = 0; + *buffer = NULL; + pr_err("%s: Kexec file buf not allocated\n", __func__); + return -EINVAL; } - file.size = segment_size; - file.read_pos = 0; - file.count = sizeof(khdr); /* reserved space */ - memset(&khdr, 0, sizeof(khdr)); khdr.version = 1; + + /* Copy as many IMA measurements list records as possible */ list_for_each_entry_rcu(qe, &ima_measurements, later) { - if (file.count < file.size) { + if (ima_kexec_file.count < ima_kexec_file.size) { khdr.count++; - ima_measurements_show(&file, qe); + ima_measurements_show(&ima_kexec_file, qe); } else { - ret = -EINVAL; + pr_err("%s: IMA log file is too big for Kexec buf\n", + __func__); break; } } - if (ret < 0) - goto out; - /* * fill in reserved space with some buffer details * (eg. version, buffer size, number of measurements) */ - khdr.buffer_size = file.count; + khdr.buffer_size = ima_kexec_file.count; if (ima_canonical_fmt) { khdr.version = cpu_to_le16(khdr.version); khdr.count = cpu_to_le64(khdr.count); khdr.buffer_size = cpu_to_le64(khdr.buffer_size); } - memcpy(file.buf, &khdr, sizeof(khdr)); + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr)); print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1, - file.buf, file.count < 100 ? file.count : 100, + ima_kexec_file.buf, ima_kexec_file.count < 100 ? + ima_kexec_file.count : 100, true); - *buffer_size = file.count; - *buffer = file.buf; -out: - if (ret == -EINVAL) - vfree(file.buf); - return ret; + *buffer_size = ima_kexec_file.count; + *buffer = ima_kexec_file.buf; + + return 0; } /* @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image) return; } - ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, - kexec_segment_size); - if (!kexec_buffer) { + ret = ima_alloc_kexec_file_buf(kexec_segment_size); + if (ret < 0) { pr_err("Not enough memory for the kexec measurement buffer.\n"); return; } + ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, + kexec_segment_size); + if (ret < 0) { + pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", + __func__, ret); + return; + } + kbuf.buffer = kexec_buffer; kbuf.bufsz = kexec_buffer_size; kbuf.memsz = kexec_segment_size;
Refactor ima_dump_measurement_list() to move the memory allocation part to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'. Make the local variable ima_kexec_file in function ima_dump_measurement_list() as local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 29 deletions(-)