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.csa...@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;
>
> [..]
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
