[PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li]

From: Paul Meyer <Paul.Meyer@microsoft.com>

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@microsoft.com>
---
 tools/hv/hv_kvp_daemon.c | 66 ++++++++----------------------------------------
 1 file changed, 10 insertions(+), 56 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index eaa3bec..2094036 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -193,11 +193,13 @@ 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));
 			exit(EXIT_FAILURE);
 		}
 
@@ -224,15 +226,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 +244,17 @@ 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);
+		kvp_file_info[i].num_records = 0;
+		kvp_update_mem_state(i);
 	}
 
 	return 0;
-- 
2.7.4

Re: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Greg KH]

On Mon, Oct 30, 2017 at 05:08:03PM -0700, Long Li wrote:
> From: Paul Meyer <Paul.Meyer@microsoft.com>
> 
> 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).

Please wrap changelogs at 72 columns like your editor asked you to...

> 
> Fix this by reading the correct number of KVP records from file each time.
> 
> Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> ---

Why is your name not also on the signed-off-by chain if you are
forwarding on a patch from someone else?

Is this patch also needed on stable kernels?

thanks,

greg k-h

RE: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li]

> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, October 31, 2017 1:43 AM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Paul Meyer
> <Paul.Meyer@microsoft.com>
> Subject: Re: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file
> 
> On Mon, Oct 30, 2017 at 05:08:03PM -0700, Long Li wrote:
> > From: Paul Meyer <Paul.Meyer@microsoft.com>
> >
> > 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).
> 
> Please wrap changelogs at 72 columns like your editor asked you to...

I will fix it.

> 
> >
> > Fix this by reading the correct number of KVP records from file each time.
> >
> > Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> > ---
> 
> Why is your name not also on the signed-off-by chain if you are forwarding on a
> patch from someone else?
> 
> Is this patch also needed on stable kernels?

I'm sending on behalf of Paul Meyer. I will add a "Reviewed-by:" tag.

Yes it should also go stable. Will send v2 to include that.

> 
> thanks,
> 
> greg k-h

Re: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Greg KH]

On Tue, Oct 31, 2017 at 06:10:00PM +0000, Long Li wrote:
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, October 31, 2017 1:43 AM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Paul Meyer
> > <Paul.Meyer@microsoft.com>
> > Subject: Re: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file
> > 
> > On Mon, Oct 30, 2017 at 05:08:03PM -0700, Long Li wrote:
> > > From: Paul Meyer <Paul.Meyer@microsoft.com>
> > >
> > > 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).
> > 
> > Please wrap changelogs at 72 columns like your editor asked you to...
> 
> I will fix it.
> 
> > 
> > >
> > > Fix this by reading the correct number of KVP records from file each time.
> > >
> > > Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> > > ---
> > 
> > Why is your name not also on the signed-off-by chain if you are forwarding on a
> > patch from someone else?
> > 
> > Is this patch also needed on stable kernels?
> 
> I'm sending on behalf of Paul Meyer. I will add a "Reviewed-by:" tag.

Sending on behalf means you should add your signed-off-by, as it is
going through you.

thanks,

greg k-h

RE: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li]

> On Tue, Oct 31, 2017 at 06:10:00PM +0000, Long Li wrote:
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Tuesday, October 31, 2017 1:43 AM
> > > To: Long Li <longli@microsoft.com>
> > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > <haiyangz@microsoft.com>; Stephen Hemminger
> > > <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> > > linux-kernel@vger.kernel.org; Paul Meyer <Paul.Meyer@microsoft.com>
> > > Subject: Re: [PATCH] hv: kvp: Avoid reading past allocated blocks
> > > from KVP file
> > >
> > > On Mon, Oct 30, 2017 at 05:08:03PM -0700, Long Li wrote:
> > > > From: Paul Meyer <Paul.Meyer@microsoft.com>
> > > >
> > > > 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).
> > >
> > > Please wrap changelogs at 72 columns like your editor asked you to...
> >
> > I will fix it.
> >
> > >
> > > >
> > > > Fix this by reading the correct number of KVP records from file each time.
> > > >
> > > > Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> > > > ---
> > >
> > > Why is your name not also on the signed-off-by chain if you are
> > > forwarding on a patch from someone else?
> > >
> > > Is this patch also needed on stable kernels?
> >
> > I'm sending on behalf of Paul Meyer. I will add a "Reviewed-by:" tag.
> 
> Sending on behalf means you should add your signed-off-by, as it is going
> through you.

Thanks. I will re-send the patch.

> 
> thanks,
> 
> greg k-h

RE: [PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li]

> From: Paul Meyer <Paul.Meyer@microsoft.com>
> 
> 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.

Please drop this patch. I have sent a v2.

> 
> Signed-off-by: Paul Meyer <Paul.Meyer@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> ---
>  tools/hv/hv_kvp_daemon.c | 66 ++++++++----------------------------------------
>  1 file changed, 10 insertions(+), 56 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c index
> eaa3bec..2094036 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -193,11 +193,13 @@ 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));
>                         exit(EXIT_FAILURE);
>                 }
> 
> @@ -224,15 +226,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 +244,17 @@ 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);
> +               kvp_file_info[i].num_records = 0;
> +               kvp_update_mem_state(i);
>         }
> 
>         return 0;
> --
> 2.7.4
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdriverdev.li
> nuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=02%7C01%7Clongli%40microsoft.com%7C3d25aed8f1a14fb966170
> 8d52091db50%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6364507
> 33196130349&sdata=7SZq7ER6YQo5ci6GmtPZUsL41g%2BERq2sswLeZNEb43k%
> 3D&reserved=0

[PATCH] hv: kvp: Avoid reading past allocated blocks from KVP file

[Long Li]

From: Paul Meyer <Paul.Meyer@microsoft.com>

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@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
---
 tools/hv/hv_kvp_daemon.c | 66 ++++++++----------------------------------------
 1 file changed, 10 insertions(+), 56 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index eaa3bec..2094036 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -193,11 +193,13 @@ 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));
 			exit(EXIT_FAILURE);
 		}
 
@@ -224,15 +226,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 +244,17 @@ 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);
+		kvp_file_info[i].num_records = 0;
+		kvp_update_mem_state(i);
 	}
 
 	return 0;
-- 
2.7.4