Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 8, 2022 10:40 am, Fabian Grünbichler wrote:
> On March 17, 2022 12:44 pm, Dominik Csapak wrote:
>> for getting multiple properties from the in memory config of the
>> guests. I added a new CSF_IPC_ call to maintain backwards compatibility.
>> 
>> It basically behaves the same as
>> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
>> instead.
>> 
>> The old way of getting a single property is now also done by
>> the new function.
>> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> 
> one more small nit, otherwise this LGTM
> 

obviously after hitting send I found another one :-P

>> ---
>> changes since v2:
>> * add 'a-z' check while parsing the properties, this way the later min/max
>>   code works as intended
>> 
>>  data/src/cfs-ipc-ops.h |   2 +
>>  data/src/server.c      |  62 +++++++++++++++
>>  data/src/status.c      | 174 ++++++++++++++++++++++++++++-------------
>>  data/src/status.h      |   3 +
>>  4 files changed, 185 insertions(+), 56 deletions(-)
>> 
>> diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
>> index 003e233..249308d 100644
>> --- a/data/src/cfs-ipc-ops.h
>> +++ b/data/src/cfs-ipc-ops.h
>> @@ -43,4 +43,6 @@
>>  
>>  #define CFS_IPC_VERIFY_TOKEN 12
>>  
>> +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
>> +
>>  #endif
>> diff --git a/data/src/server.c b/data/src/server.c
>> index 549788a..b9394c8 100644
>> --- a/data/src/server.c
>> +++ b/data/src/server.c
>> @@ -89,6 +89,13 @@ typedef struct {
>>  	char property[];
>>  } cfs_guest_config_propery_get_request_header_t;
>>  
>> +typedef struct {
>> +	struct qb_ipc_request_header req_header;
>> +	uint32_t vmid;
>> +	uint8_t num_props;
>> +	char props[]; /* list of \0 terminated properties */
>> +} cfs_guest_config_properties_get_request_header_t;
>> +
>>  typedef struct {
>>  	struct qb_ipc_request_header req_header;
>>  	char token[];
>> @@ -348,6 +355,61 @@ static int32_t s1_msg_process_fn(
>>  
>>  			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
>>  		}
>> +	} else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
>> +
>> +		cfs_guest_config_properties_get_request_header_t *rh =
>> +			(cfs_guest_config_properties_get_request_header_t *) data;
>> +
>> +		int propslen = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);

so request_size is int32_t, and the offset is 32 + 32 (qb header) + 32 + 
8 = 13 bytes, so propslen will always fit in an int and be positive.

>> +
>> +		result = 0;
>> +		if (rh->vmid < 100 && rh->vmid != 0) {
>> +			cfs_debug("vmid out of range %u", rh->vmid);
>> +			result = -EINVAL;
>> +		} else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
>> +			result = -ENOENT;
>> +		} else if (propslen <= 1) {
>> +			cfs_debug("propslen <= 1, %d", propslen);
>> +			result = -EINVAL;
>> +		} else {
>> +			const char **properties = malloc(sizeof(char*) * rh->num_props);
>> +			char *current = (rh->props);
>> +			int remaining = propslen;

remaining starts positive

>> +			for (uint8_t i = 0; i < rh->num_props; i++) {
>> +			    uint8_t proplen = strnlen(current, remaining);

this ain't no uint8_t ;) so this takes a size_t (positive int is fine 
accordingly) and returns one. a property can be longer than 255 chars 
though, which makes the assignment overflow and gives us a 
wrong/truncated proplen

>> +			    if (proplen == 0) {

could trigger this if the overflow aligns right.

>> +				cfs_debug("property length 0");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    if (proplen == remaining) {

can't be triggered by overflow since proplen must always be smaller than 
remaining

>> +				cfs_debug("property not \\0 terminated");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    if (current[0] < 'a' || current[0] > 'z') {

this could be triggered depending on how exactly proplen overflows / the 
property gets truncated

>> +				cfs_debug("property does not start with [a-z]");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    properties[i] = current;
>> +			    current[proplen] = '\0'; // ensure property is 0 terminated

truncates an overly long property

>> +			    remaining -= (proplen + 1);
>> +			    current += proplen + 1;

and we start the next iteration at the truncated position

>> +			}
>> +
>> +			if (remaining != 0) {

which likely makes us end up here (except if the msg was malformed with 
not-null-terminated properties, in which case they might align again and 
just their boundaries are wrong ;))

given that we assume remaining remains positive, and barring further 
future bugs that invariant should hold (we start off positive, in each 
iteration proplen must be < remaining to reach the subtraction which 
therefore at most reduces remaining to 0), we could switch both 
remaining and proplen to size_t ?

or if the/a limit on property length is actually desired, we should 
explicitly check for that (propslen <= limit * count at the start, and 
then in the loop body still get a size_t from strnlen and check that the 
result is < limit)

> 
>> +			    cfs_debug("leftover data after parsing %ul properties", rh->num_props);
>> +			    result = -EINVAL;
>> +			}
>> +
>> +			if (result == 0) {
>> +			    cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
>> +			    result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
>> +			}
>> +
>> +			free(properties);
>> +		}
>>  	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
>>  
>>  		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
> 
> [..]
>