diff mbox series

[v10,2/8] ima: define and call ima_alloc_kexec_file_buf()

Message ID 20250318010448.954-3-chenste@linux.microsoft.com (mailing list archive)
State New
Headers show
Series ima: kexec: measure events between kexec load and execute | expand

Commit Message

steven chen March 18, 2025, 1:04 a.m. UTC
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'.

This patch includes the following changes:
 - Refactor ima_dump_measurement_list() to move the memory allocation
   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 ima_dump_measurement_list()
   a 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>
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 67 +++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 11 deletions(-)

Comments

Baoquan He March 19, 2025, 8:09 a.m. UTC | #1
On 03/17/25 at 06:04pm, steven chen wrote:
> 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'.
> 
> This patch includes the following changes:
>  - Refactor ima_dump_measurement_list() to move the memory allocation
>    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 ima_dump_measurement_list()
>    a 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.

We may not need above details about code change because it's not so
difficult to get them from patch.

> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> Signed-off-by: steven chen <chenste@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_kexec.c | 67 +++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 8567619889d1..45170e283272 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,6 +15,48 @@
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
> +static struct seq_file ima_kexec_file;
> +
> +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);
> +}
> +
> +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)
> +		goto out;
> +
> +	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;
> +
> +out:
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
> +
> +	return 0;
> +}
> +
>  /*
>   * Copy the measurement list to the allocated memory
>   * compare the size of IMA measurement list with the size of the allocated memory
> @@ -26,23 +68,16 @@
>  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  				     unsigned long segment_size)
>  {
> -	struct seq_file ima_kexec_file;
>  	struct ima_queue_entry *qe;
>  	struct ima_kexec_hdr khdr;
>  	int ret = 0;
>  
>  	/* segment size can't change between kexec load and execute */
> -	ima_kexec_file.buf = vmalloc(segment_size);
>  	if (!ima_kexec_file.buf) {
> -		ret = -ENOMEM;
> -		goto out;
> +		pr_err("Kexec file buf not allocated\n");
> +		return -EINVAL;
>  	}
>  
> -	ima_kexec_file.file = NULL;
> -	ima_kexec_file.size = segment_size;
> -	ima_kexec_file.read_pos = 0;
> -	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
> -
>  	memset(&khdr, 0, sizeof(khdr));
>  	khdr.version = 1;
>  	/* This is an append-only list, no need to hold the RCU read lock */
> @@ -79,8 +114,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  	*buffer_size = ima_kexec_file.count;
>  	*buffer = ima_kexec_file.buf;
>  out:
> -	if (ret == -EINVAL)
> -		vfree(ima_kexec_file.buf);
>  	return ret;
>  }
>  
> @@ -119,6 +152,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>  		return;
>  	}
>  
> +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> +	if (ret < 0) {
> +		pr_err("Not enough memory for the kexec measurement buffer.\n");
> +		return;
> +	}
> +
>  	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>  				  kexec_segment_size);
>  	if (!kexec_buffer) {
> @@ -140,6 +179,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	image->ima_buffer_size = kexec_segment_size;
>  	image->ima_buffer = kexec_buffer;
>  
> +	/*
> +	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> +	 * and it will vfree() that buffer.
> +	 */
> +	ima_reset_kexec_file(&ima_kexec_file);

I can't see why we need call ima_reset_kexec_file() here. If we need
reuse the buffer, we will reset the needed fields at the end of
ima_alloc_kexec_file_buf(). Not sure if I miss anything.

static int ima_alloc_kexec_file_buf(size_t segment_size)
{
......
out:
        ima_kexec_file.read_pos = 0;
        ima_kexec_file.count = sizeof(struct ima_kexec_hdr);    /* reserved space */

        return 0;
}
Mimi Zohar March 19, 2025, 4:27 p.m. UTC | #2
On Wed, 2025-03-19 at 16:09 +0800, Baoquan He wrote:
> On 03/17/25 at 06:04pm, steven chen wrote:
> > 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'.
> > 
> > This patch includes the following changes:
> >  - Refactor ima_dump_measurement_list() to move the memory allocation
> >    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 ima_dump_measurement_list()
> >    a 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.
> 
> We may not need above details about code change because it's not so
> difficult to get them from patch.

Agreed.  The changes don't even reflect the current patch.  Please remove the
entire section.

> 
> > 
> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > ---
> >  security/integrity/ima/ima_kexec.c | 67 +++++++++++++++++++++++++-----
> >  1 file changed, 56 insertions(+), 11 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 8567619889d1..45170e283272 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -15,6 +15,48 @@
> >  #include "ima.h"
> >  
> >  #ifdef CONFIG_IMA_KEXEC
> > +static struct seq_file ima_kexec_file;
> > +
> > +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);
> > +}
> > +
> > +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)
> > +		goto out;

The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets
ima_kexec_file.buf() hiding the fact that the above test always fails and falls
through.  As a result, 'buf' is always being re-allocated.

> > +
> > +	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;
> > +
> > +out:
> > +	ima_kexec_file.read_pos = 0;
> > +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Copy the measurement list to the allocated memory
> >   * compare the size of IMA measurement list with the size of the allocated memory
> > @@ -26,23 +68,16 @@
> >  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> >  				     unsigned long segment_size)
> >  {
> > -	struct seq_file ima_kexec_file;
> >  	struct ima_queue_entry *qe;
> >  	struct ima_kexec_hdr khdr;
> >  	int ret = 0;
> >  
> >  	/* segment size can't change between kexec load and execute */
> > -	ima_kexec_file.buf = vmalloc(segment_size);
> >  	if (!ima_kexec_file.buf) {
> > -		ret = -ENOMEM;
> > -		goto out;
> > +		pr_err("Kexec file buf not allocated\n");
> > +		return -EINVAL;
> >  	}
> >  
> > -	ima_kexec_file.file = NULL;
> > -	ima_kexec_file.size = segment_size;
> > -	ima_kexec_file.read_pos = 0;
> > -	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
> > -
> >  	memset(&khdr, 0, sizeof(khdr));
> >  	khdr.version = 1;
> >  	/* This is an append-only list, no need to hold the RCU read lock */
> > @@ -79,8 +114,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> >  	*buffer_size = ima_kexec_file.count;
> >  	*buffer = ima_kexec_file.buf;
> >  out:
> > -	if (ret == -EINVAL)
> > -		vfree(ima_kexec_file.buf);
> >  	return ret;
> >  }
> >  
> > @@ -119,6 +152,12 @@ void ima_add_kexec_buffer(struct kimage *image)
> >  		return;
> >  	}
> >  
> > +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> > +	if (ret < 0) {
> > +		pr_err("Not enough memory for the kexec measurement buffer.\n");
> > +		return;
> > +	}
> > +
> >  	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> >  				  kexec_segment_size);
> >  	if (!kexec_buffer) {
> > @@ -140,6 +179,12 @@ void ima_add_kexec_buffer(struct kimage *image)
> >  	image->ima_buffer_size = kexec_segment_size;
> >  	image->ima_buffer = kexec_buffer;
> >  
> > +	/*
> > +	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> > +	 * and it will vfree() that buffer.
> > +	 */
> > +	ima_reset_kexec_file(&ima_kexec_file);
> 
> I can't see why we need call ima_reset_kexec_file() here. If we need
> reuse the buffer, we will reset the needed fields at the end of
> ima_alloc_kexec_file_buf(). Not sure if I miss anything.

Without ima_reset_kexec_file(), calling 'kexec load' consecutively without
"kexec -s -e" in between fails.

# kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
# kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
Killed

As mentioned above, the call to ima_reset_kexec_file() resets
ima_kexec_file.buf, so the segment size test always fails and the memory is
being allocated.

Mimi

> 
> static int ima_alloc_kexec_file_buf(size_t segment_size)
> {
> ......
> out:
>         ima_kexec_file.read_pos = 0;
>         ima_kexec_file.count = sizeof(struct ima_kexec_hdr);    /* reserved space */
> 
>         return 0;
> }
> 
>
Baoquan He March 20, 2025, 1:51 a.m. UTC | #3
On 03/19/25 at 12:27pm, Mimi Zohar wrote:
> On Wed, 2025-03-19 at 16:09 +0800, Baoquan He wrote:
> > On 03/17/25 at 06:04pm, steven chen wrote:
> > > 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'.
> > > 
> > > This patch includes the following changes:
> > >  - Refactor ima_dump_measurement_list() to move the memory allocation
> > >    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 ima_dump_measurement_list()
> > >    a 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.
> > 
> > We may not need above details about code change because it's not so
> > difficult to get them from patch.
> 
> Agreed.  The changes don't even reflect the current patch.  Please remove the
> entire section.
> 
> > 
> > > 
> > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > > ---
> > >  security/integrity/ima/ima_kexec.c | 67 +++++++++++++++++++++++++-----
> > >  1 file changed, 56 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > > index 8567619889d1..45170e283272 100644
> > > --- a/security/integrity/ima/ima_kexec.c
> > > +++ b/security/integrity/ima/ima_kexec.c
> > > @@ -15,6 +15,48 @@
> > >  #include "ima.h"
> > >  
> > >  #ifdef CONFIG_IMA_KEXEC
> > > +static struct seq_file ima_kexec_file;
> > > +
> > > +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);
> > > +}
> > > +
> > > +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)
> > > +		goto out;
> 
> The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets
> ima_kexec_file.buf() hiding the fact that the above test always fails and falls
> through.  As a result, 'buf' is always being re-allocated.
> 
> > > +
> > > +	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;
> > > +
> > > +out:
> > > +	ima_kexec_file.read_pos = 0;
> > > +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * Copy the measurement list to the allocated memory
> > >   * compare the size of IMA measurement list with the size of the allocated memory
> > > @@ -26,23 +68,16 @@
> > >  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> > >  				     unsigned long segment_size)
> > >  {
> > > -	struct seq_file ima_kexec_file;
> > >  	struct ima_queue_entry *qe;
> > >  	struct ima_kexec_hdr khdr;
> > >  	int ret = 0;
> > >  
> > >  	/* segment size can't change between kexec load and execute */
> > > -	ima_kexec_file.buf = vmalloc(segment_size);
> > >  	if (!ima_kexec_file.buf) {
> > > -		ret = -ENOMEM;
> > > -		goto out;
> > > +		pr_err("Kexec file buf not allocated\n");
> > > +		return -EINVAL;
> > >  	}
> > >  
> > > -	ima_kexec_file.file = NULL;
> > > -	ima_kexec_file.size = segment_size;
> > > -	ima_kexec_file.read_pos = 0;
> > > -	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
> > > -
> > >  	memset(&khdr, 0, sizeof(khdr));
> > >  	khdr.version = 1;
> > >  	/* This is an append-only list, no need to hold the RCU read lock */
> > > @@ -79,8 +114,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> > >  	*buffer_size = ima_kexec_file.count;
> > >  	*buffer = ima_kexec_file.buf;
> > >  out:
> > > -	if (ret == -EINVAL)
> > > -		vfree(ima_kexec_file.buf);
> > >  	return ret;
> > >  }
> > >  
> > > @@ -119,6 +152,12 @@ void ima_add_kexec_buffer(struct kimage *image)
> > >  		return;
> > >  	}
> > >  
> > > +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> > > +	if (ret < 0) {
> > > +		pr_err("Not enough memory for the kexec measurement buffer.\n");
> > > +		return;
> > > +	}
> > > +
> > >  	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> > >  				  kexec_segment_size);
> > >  	if (!kexec_buffer) {
> > > @@ -140,6 +179,12 @@ void ima_add_kexec_buffer(struct kimage *image)
> > >  	image->ima_buffer_size = kexec_segment_size;
> > >  	image->ima_buffer = kexec_buffer;
> > >  
> > > +	/*
> > > +	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> > > +	 * and it will vfree() that buffer.
> > > +	 */
> > > +	ima_reset_kexec_file(&ima_kexec_file);
> > 
> > I can't see why we need call ima_reset_kexec_file() here. If we need
> > reuse the buffer, we will reset the needed fields at the end of
> > ima_alloc_kexec_file_buf(). Not sure if I miss anything.
> 
> Without ima_reset_kexec_file(), calling 'kexec load' consecutively without
> "kexec -s -e" in between fails.
> 
> # kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
> # kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
> Killed
> 
> As mentioned above, the call to ima_reset_kexec_file() resets
> ima_kexec_file.buf, so the segment size test always fails and the memory is
> being allocated.

Indeed, I didn't realize this.
Mimi Zohar March 20, 2025, 1:06 p.m. UTC | #4
On Thu, 2025-03-20 at 09:51 +0800, Baoquan He wrote:
> > > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > > > index 8567619889d1..45170e283272 100644
> > > > --- a/security/integrity/ima/ima_kexec.c
> > > > +++ b/security/integrity/ima/ima_kexec.c
> > > > @@ -15,6 +15,48 @@
> > > >   #include "ima.h"
> > > >   
> > > >   #ifdef CONFIG_IMA_KEXEC
> > > > +static struct seq_file ima_kexec_file;
> > > > +
> > > > +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);
> > > > +}
> > > > +
> > > > +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)
> > > > +		goto out;
> > 
> > The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets
> > ima_kexec_file.buf() hiding the fact that the above test always fails and falls
> > through.  As a result, 'buf' is always being re-allocated.

Hi Steven,

[Reiterating the comment in the "ima: kexec: move IMA log copy from kexec load
to execute" thread, here, for completeness.]

Instead of adding and then removing the ima_reset_kexec_file() call from
ima_add_kexec_buffer(), defer adding the segment size test to when it is
actually possible for the segment size to change. Please make the segment size
test as a separate patch.

ima_reset_kexec_file() will then only be called by ima_free_kexec_file_buf(). 
Inline the ima_reset_kexec_file() code in ima_free_kexec_file_buf().

thanks,

Mimi
steven chen March 21, 2025, 4:18 p.m. UTC | #5
On 3/20/2025 6:06 AM, Mimi Zohar wrote:
> On Thu, 2025-03-20 at 09:51 +0800, Baoquan He wrote:
>>>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>>>> index 8567619889d1..45170e283272 100644
>>>>> --- a/security/integrity/ima/ima_kexec.c
>>>>> +++ b/security/integrity/ima/ima_kexec.c
>>>>> @@ -15,6 +15,48 @@
>>>>>    #include "ima.h"
>>>>>    
>>>>>    #ifdef CONFIG_IMA_KEXEC
>>>>> +static struct seq_file ima_kexec_file;
>>>>> +
>>>>> +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);
>>>>> +}
>>>>> +
>>>>> +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)
>>>>> +		goto out;
>>> The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets
>>> ima_kexec_file.buf() hiding the fact that the above test always fails and falls
>>> through.  As a result, 'buf' is always being re-allocated.
> Hi Steven,
>
> [Reiterating the comment in the "ima: kexec: move IMA log copy from kexec load
> to execute" thread, here, for completeness.]
>
> Instead of adding and then removing the ima_reset_kexec_file() call from
> ima_add_kexec_buffer(), defer adding the segment size test to when it is
> actually possible for the segment size to change. Please make the segment size
> test as a separate patch.
>
> ima_reset_kexec_file() will then only be called by ima_free_kexec_file_buf().
> Inline the ima_reset_kexec_file() code in ima_free_kexec_file_buf().
>
> thanks,
>
> Mimi

Hi Mimi,

I will update in next version.

Thanks,

Steven
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 8567619889d1..45170e283272 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -15,6 +15,48 @@ 
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+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);
+}
+
+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)
+		goto out;
+
+	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;
+
+out:
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+
+	return 0;
+}
+
 /*
  * Copy the measurement list to the allocated memory
  * compare the size of IMA measurement list with the size of the allocated memory
@@ -26,23 +68,16 @@ 
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
-	struct seq_file ima_kexec_file;
 	struct ima_queue_entry *qe;
 	struct ima_kexec_hdr khdr;
 	int ret = 0;
 
 	/* segment size can't change between kexec load and execute */
-	ima_kexec_file.buf = vmalloc(segment_size);
 	if (!ima_kexec_file.buf) {
-		ret = -ENOMEM;
-		goto out;
+		pr_err("Kexec file buf not allocated\n");
+		return -EINVAL;
 	}
 
-	ima_kexec_file.file = NULL;
-	ima_kexec_file.size = segment_size;
-	ima_kexec_file.read_pos = 0;
-	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
-
 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
 	/* This is an append-only list, no need to hold the RCU read lock */
@@ -79,8 +114,6 @@  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	*buffer_size = ima_kexec_file.count;
 	*buffer = ima_kexec_file.buf;
 out:
-	if (ret == -EINVAL)
-		vfree(ima_kexec_file.buf);
 	return ret;
 }
 
@@ -119,6 +152,12 @@  void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
+	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
+	if (ret < 0) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		return;
+	}
+
 	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
 				  kexec_segment_size);
 	if (!kexec_buffer) {
@@ -140,6 +179,12 @@  void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 
+	/*
+	 * kexec owns kexec_buffer after kexec_add_buffer() is called
+	 * and it will vfree() that buffer.
+	 */
+	ima_reset_kexec_file(&ima_kexec_file);
+
 	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);
 }