Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote:
> Hi Tushar,
> 
> According to Documentation/process/submitting-patches.rst, the subject
> line should be between 70-75 characters.
> 
> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
> 
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
> > IMA allocates memory and dumps the measurement during kexec soft reboot
> > as a single function call ima_dump_measurement_list().  It gets called
> > during kexec 'load' operation.  It results in the IMA measurements
> > between the window of kexec 'load' and 'execute' getting dropped when the
> > system boots into the new Kernel.  One of the kexec requirements is the
> > segment size cannot change between the 'load' and the 'execute'.
> > Therefore, to address this problem, ima_dump_measurement_list() needs
> > to be refactored to allocate the memory at kexec 'load', and dump the
> > measurements at kexec 'execute'.  The function that allocates the memory
> > should handle the scenario where the kexec load is called multiple times
> 
> The above pragraph is unnecessary.
> 
> > Refactor ima_dump_measurement_list() to move the memory allocation part
> > to a separate function ima_alloc_kexec_buf() to allocate buffer of size
> > 'kexec_segment_size' at kexec 'load'.  Make the local variables in
> > function ima_dump_measurement_list() global, so that they can be accessed
> > from ima_alloc_kexec_buf().  Make necessary changes to the function
> > ima_add_kexec_buffer() to call the above two functions.
> 
> Fix the wording based on the suggested changes below.
> 
> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> - Before re-posting this patch set, verify there aren't any
> "checkpatch.pl --strict" issues.
> - After applying each patch, compile the kernel and verify it still
> works.

Doing this will detect whether or not the patch set is bisect safe.

> > ---
> >  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
> >  1 file changed, 93 insertions(+), 33 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 419dc405c831..307e07991865 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -15,61 +15,114 @@
> >  #include "ima.h"
> >  
> >  #ifdef CONFIG_IMA_KEXEC
> > +struct seq_file ima_kexec_file;
> 
> Define "ima_kexec_file" as static since it only used in this file.  
> Since the variable does not need to be global, is there still a reason
> for changing its name?   Minimize code change.

Adding "static" would make ima_kexec_file a global static variable. 
Please ignore my comment about reverting the variable name change.

Mimi

> 
> > +struct ima_kexec_hdr ima_khdr;
> > +
> > +void ima_clear_kexec_file(void)
> 
> The opposite of "set" is "clear" (e.g. set_git, clear_bit).  The
> opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf,
> ima_free_kexec_buf)
> 
> > +{
> > +	vfree(ima_kexec_file.buf);
> > +	ima_kexec_file.buf = NULL;
> > +	ima_kexec_file.size = 0;
> > +	ima_kexec_file.read_pos = 0;
> > +	ima_kexec_file.count = 0;
> > +}
> > +
> > +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> > +{
> > +	if ((kexec_segment_size == 0) ||
> > +	    (kexec_segment_size == ULONG_MAX) ||
> > +	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> > +		pr_err("%s: Invalid segment size for kexec: %zu\n",
> > +			__func__, kexec_segment_size);
> > +		return -EINVAL;
> > +	}
> 
> Please avoid code duplication.  If moving the test here, then remove it
> from its original place.
> 
> > +
> > +	/*
> > +	 * If kexec load was called before, clear the existing buffer
> > +	 * before allocating a new one
> > +	 */
> 
> > +	if (ima_kexec_file.buf)
> > +		ima_clear_kexec_file();
> 
> Perhaps instead of always freeing the buffer, check and see whether the
> size has changed.  If it hasn't changed, then simply return.  If it has
> changed, perhaps use realloc().  Possible wording:
> 
> "Kexec may be called multiple times.  Free and re-alloc the buffer if
> the size changed."
> 
> > +
> > +	/* segment size can't change between kexec load and execute */
> > +	ima_kexec_file.buf = vmalloc(kexec_segment_size);
> > +	if (!ima_kexec_file.buf) {
> > +		pr_err("%s: No memory for ima kexec measurement buffer\n",
> > +			__func__);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ima_kexec_file.size = kexec_segment_size;
> > +	ima_kexec_file.read_pos = 0;
> > +	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
> > +
> > +	memset(&ima_khdr, 0, sizeof(ima_khdr));
> > +	ima_khdr.version = 1;
> > +
> > +	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) {
> > +		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 */
> > +	/*
> > +	 * Ensure the kexec buffer is large enough to hold ima_khdr
> > +	 */
> > +	if (ima_kexec_file.size < sizeof(ima_khdr)) {
> > +		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
> > +			__func__);
> > +		ima_clear_kexec_file();
> > +		return -ENOMEM;
> > +	}
> 
> Is this necessary?
>  
> > -	memset(&khdr, 0, sizeof(khdr));
> > -	khdr.version = 1;
> > +	/*
> > +	 * If we reach here, then there is enough memory
> > +	 * of size kexec_segment_size in ima_kexec_file.buf
> > +	 * to copy at least partial IMA log.
> > +	 * Make best effort to copy as many IMA measurements
> > +	 * as possible.
> > +	 */
> 
> Suggestion:
> 
> /* Copy as many IMA measurements list records as possible */
> 
> >  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> > -		if (file.count < file.size) {
> > -			khdr.count++;
> > -			ima_measurements_show(&file, qe);
> > +		if (ima_kexec_file.count < ima_kexec_file.size) {
> > +			ima_khdr.count++;
> > +			ima_measurements_show(&ima_kexec_file, qe);
> >  		} else {
> > -			ret = -EINVAL;
> > +			ret = EFBIG;
> > +			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;
> > +	ima_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);
> > +		ima_khdr.version = cpu_to_le16(ima_khdr.version);
> > +		ima_khdr.count = cpu_to_le64(ima_khdr.count);
> > +		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
> >  	}
> > -	memcpy(file.buf, &khdr, sizeof(khdr));
> > +	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
> > +	*buffer_size = ima_kexec_file.count;
> > +	*buffer = ima_kexec_file.buf;
> > +
> >  	return ret;
> >  }
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec