ACK: [SRU][Artful][PATCH 1/1] hv: kvp: Avoid reading past allocated blocks from KVP file

[Stefan Bader]

On 15.03.2018 20:44, Joseph Salisbury wrote:
> From: Paul Meyer <Paul.Meyer at microsoft.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1750349
> 
> While reading in more than one block (50) of KVP records, the allocation
> goes per block, but the reads used the total number of allocated records
> (without resetting the pointer/stream). This causes the records buffer to
> overrun when the refresh reads more than one block over the previous
> capacity (e.g. reading more than 100 KVP records whereas the in-memory
> database was empty before).
> 
> Fix this by reading the correct number of KVP records from file each time.
> 
> Signed-off-by: Paul Meyer <Paul.Meyer at microsoft.com>
> Signed-off-by: Long Li <longli at microsoft.com>
> Cc: stable at vger.kernel.org
> Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry picked from commit 297d6b6e56c2977fc504c61bbeeaa21296923f89)
> Signed-off-by: Joseph Salisbury <joseph.salisbury at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>

> ---
>  tools/hv/hv_kvp_daemon.c | 70 ++++++++++--------------------------------------
>  1 file changed, 14 insertions(+), 56 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 88b20e0..2357a6c 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -193,11 +193,14 @@ static void kvp_update_mem_state(int pool)
>  	for (;;) {
>  		readp = &record[records_read];
>  		records_read += fread(readp, sizeof(struct kvp_record),
> -					ENTRIES_PER_BLOCK * num_blocks,
> -					filep);
> +				ENTRIES_PER_BLOCK * num_blocks - records_read,
> +				filep);
>  
>  		if (ferror(filep)) {
> -			syslog(LOG_ERR, "Failed to read file, pool: %d", pool);
> +			syslog(LOG_ERR,
> +				"Failed to read file, pool: %d; error: %d %s",
> +				 pool, errno, strerror(errno));
> +			kvp_release_lock(pool);
>  			exit(EXIT_FAILURE);
>  		}
>  
> @@ -210,6 +213,7 @@ static void kvp_update_mem_state(int pool)
>  
>  			if (record == NULL) {
>  				syslog(LOG_ERR, "malloc failed");
> +				kvp_release_lock(pool);
>  				exit(EXIT_FAILURE);
>  			}
>  			continue;
> @@ -224,15 +228,11 @@ static void kvp_update_mem_state(int pool)
>  	fclose(filep);
>  	kvp_release_lock(pool);
>  }
> +
>  static int kvp_file_init(void)
>  {
>  	int  fd;
> -	FILE *filep;
> -	size_t records_read;
>  	char *fname;
> -	struct kvp_record *record;
> -	struct kvp_record *readp;
> -	int num_blocks;
>  	int i;
>  	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
>  
> @@ -246,61 +246,19 @@ static int kvp_file_init(void)
>  
>  	for (i = 0; i < KVP_POOL_COUNT; i++) {
>  		fname = kvp_file_info[i].fname;
> -		records_read = 0;
> -		num_blocks = 1;
>  		sprintf(fname, "%s/.kvp_pool_%d", KVP_CONFIG_LOC, i);
>  		fd = open(fname, O_RDWR | O_CREAT | O_CLOEXEC, 0644 /* rw-r--r-- */);
>  
>  		if (fd == -1)
>  			return 1;
>  
> -
> -		filep = fopen(fname, "re");
> -		if (!filep) {
> -			close(fd);
> -			return 1;
> -		}
> -
> -		record = malloc(alloc_unit * num_blocks);
> -		if (record == NULL) {
> -			fclose(filep);
> -			close(fd);
> -			return 1;
> -		}
> -		for (;;) {
> -			readp = &record[records_read];
> -			records_read += fread(readp, sizeof(struct kvp_record),
> -					ENTRIES_PER_BLOCK,
> -					filep);
> -
> -			if (ferror(filep)) {
> -				syslog(LOG_ERR, "Failed to read file, pool: %d",
> -				       i);
> -				exit(EXIT_FAILURE);
> -			}
> -
> -			if (!feof(filep)) {
> -				/*
> -				 * We have more data to read.
> -				 */
> -				num_blocks++;
> -				record = realloc(record, alloc_unit *
> -						num_blocks);
> -				if (record == NULL) {
> -					fclose(filep);
> -					close(fd);
> -					return 1;
> -				}
> -				continue;
> -			}
> -			break;
> -		}
>  		kvp_file_info[i].fd = fd;
> -		kvp_file_info[i].num_blocks = num_blocks;
> -		kvp_file_info[i].records = record;
> -		kvp_file_info[i].num_records = records_read;
> -		fclose(filep);
> -
> +		kvp_file_info[i].num_blocks = 1;
> +		kvp_file_info[i].records = malloc(alloc_unit);
> +		if (kvp_file_info[i].records == NULL)
> +			return 1;
> +		kvp_file_info[i].num_records = 0;
> +		kvp_update_mem_state(i);
>  	}
>  
>  	return 0;
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180320/c1dcb84e/attachment.sig>