| Message ID | 20210121173003.18324-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ima: Free IMA measurement buffer on error | expand |
On 2021-01-21 09:30:02, Lakshmi Ramasubramanian wrote: > IMA allocates kernel virtual memory to carry forward the measurement > list, from the current kernel to the next kernel on kexec system call, > in ima_add_kexec_buffer() function. In error code paths this memory > is not freed resulting in memory leak. > > Free the memory allocated for the IMA measurement list in > the error code paths in ima_add_kexec_buffer() function. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Suggested-by: Tyler Hicks <tyhicks@linux.microsoft.com> > Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list") Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com> Tyler > --- > security/integrity/ima/ima_kexec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 121de3e04af2..212145008a01 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -119,12 +119,14 @@ void ima_add_kexec_buffer(struct kimage *image) > ret = kexec_add_buffer(&kbuf); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > + vfree(kexec_buffer); > return; > } > > ret = arch_ima_add_kexec_buffer(image, kbuf.mem, kexec_segment_size); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > + vfree(kexec_buffer); > return; > } > > -- > 2.30.0 >
Hi Lakshmi, Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > IMA allocates kernel virtual memory to carry forward the measurement > list, from the current kernel to the next kernel on kexec system call, > in ima_add_kexec_buffer() function. In error code paths this memory > is not freed resulting in memory leak. > > Free the memory allocated for the IMA measurement list in > the error code paths in ima_add_kexec_buffer() function. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Suggested-by: Tyler Hicks <tyhicks@linux.microsoft.com> > Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list") > --- > security/integrity/ima/ima_kexec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 121de3e04af2..212145008a01 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -119,12 +119,14 @@ void ima_add_kexec_buffer(struct kimage *image) > ret = kexec_add_buffer(&kbuf); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > + vfree(kexec_buffer); > return; > } This is a good catch. > > ret = arch_ima_add_kexec_buffer(image, kbuf.mem, kexec_segment_size); > if (ret) { > pr_err("Error passing over kexec measurement buffer.\n"); > + vfree(kexec_buffer); > return; > } But this would cause problems, because the buffer is still there in the kimage and would cause kimage_load_segment() to access invalid memory. There's no function to undo a kexec_add_buffer() to avoid this problem, so I'd suggest just accepting the leak in this case. Fortunately, the current implementations of arch_ima_add_kexec_buffer() are very simple and cannot fail, so this is a theoretical problem.
On 1/22/21 2:30 PM, Thiago Jung Bauermann wrote: > > Hi Lakshmi, > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > >> IMA allocates kernel virtual memory to carry forward the measurement >> list, from the current kernel to the next kernel on kexec system call, >> in ima_add_kexec_buffer() function. In error code paths this memory >> is not freed resulting in memory leak. >> >> Free the memory allocated for the IMA measurement list in >> the error code paths in ima_add_kexec_buffer() function. >> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> Suggested-by: Tyler Hicks <tyhicks@linux.microsoft.com> >> Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list") >> --- >> security/integrity/ima/ima_kexec.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 121de3e04af2..212145008a01 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -119,12 +119,14 @@ void ima_add_kexec_buffer(struct kimage *image) >> ret = kexec_add_buffer(&kbuf); >> if (ret) { >> pr_err("Error passing over kexec measurement buffer.\n"); >> + vfree(kexec_buffer); >> return; >> } > > This is a good catch. Thanks. > >> >> ret = arch_ima_add_kexec_buffer(image, kbuf.mem, kexec_segment_size); >> if (ret) { >> pr_err("Error passing over kexec measurement buffer.\n"); >> + vfree(kexec_buffer); >> return; >> } > > But this would cause problems, because the buffer is still there in the > kimage and would cause kimage_load_segment() to access invalid memory. > > There's no function to undo a kexec_add_buffer() to avoid this problem, > so I'd suggest just accepting the leak in this case. Fortunately, the > current implementations of arch_ima_add_kexec_buffer() are very simple > and cannot fail, so this is a theoretical problem. > Agreed. I'll post a new patch with the above change removed. thanks, -lakshmi
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 121de3e04af2..212145008a01 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -119,12 +119,14 @@ void ima_add_kexec_buffer(struct kimage *image) ret = kexec_add_buffer(&kbuf); if (ret) { pr_err("Error passing over kexec measurement buffer.\n"); + vfree(kexec_buffer); return; } ret = arch_ima_add_kexec_buffer(image, kbuf.mem, kexec_segment_size); if (ret) { pr_err("Error passing over kexec measurement buffer.\n"); + vfree(kexec_buffer); return; }
IMA allocates kernel virtual memory to carry forward the measurement list, from the current kernel to the next kernel on kexec system call, in ima_add_kexec_buffer() function. In error code paths this memory is not freed resulting in memory leak. Free the memory allocated for the IMA measurement list in the error code paths in ima_add_kexec_buffer() function. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Suggested-by: Tyler Hicks <tyhicks@linux.microsoft.com> Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list") --- security/integrity/ima/ima_kexec.c | 2 ++ 1 file changed, 2 insertions(+)