From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 98F76BE36 for ; Fri, 8 Apr 2022 11:01:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8BB5DCC79 for ; Fri, 8 Apr 2022 11:01:54 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id B0247CC6F for ; Fri, 8 Apr 2022 11:01:53 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 791AE41C87 for ; Fri, 8 Apr 2022 11:01:53 +0200 (CEST) Date: Fri, 08 Apr 2022 11:01:47 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220317114415.3161589-1-d.csapak@proxmox.com> <1649406844.4azt6nsvob.astroid@nora.none> In-Reply-To: <1649406844.4azt6nsvob.astroid@nora.none> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1649407522.hmxw0c6ppz.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.175 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Apr 2022 09:01:54 -0000 On April 8, 2022 10:40 am, Fabian Gr=C3=BCnbichler 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. >>=20 >> It basically behaves the same as >> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties >> instead. >>=20 >> The old way of getting a single property is now also done by >> the new function. >>=20 >> Signed-off-by: Dominik Csapak >=20 > one more small nit, otherwise this LGTM >=20 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/m= ax >> code works as intended >>=20 >> 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(-) >>=20 >> 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 @@ >> =20 >> #define CFS_IPC_VERIFY_TOKEN 12 >> =20 >> +#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; >> =20 >> +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( >> =20 >> result =3D cfs_create_guest_conf_property_msg(outbuf, memdb, rh->pro= perty, rh->vmid); >> } >> + } else if (request_id =3D=3D CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) { >> + >> + cfs_guest_config_properties_get_request_header_t *rh =3D >> + (cfs_guest_config_properties_get_request_header_t *) data; >> + >> + int propslen =3D request_size - G_STRUCT_OFFSET(cfs_guest_config_prop= erties_get_request_header_t, props); so request_size is int32_t, and the offset is 32 + 32 (qb header) + 32 +=20 8 =3D 13 bytes, so propslen will always fit in an int and be positive. >> + >> + result =3D 0; >> + if (rh->vmid < 100 && rh->vmid !=3D 0) { >> + cfs_debug("vmid out of range %u", rh->vmid); >> + result =3D -EINVAL; >> + } else if (rh->vmid >=3D 100 && !vmlist_vm_exists(rh->vmid)) { >> + result =3D -ENOENT; >> + } else if (propslen <=3D 1) { >> + cfs_debug("propslen <=3D 1, %d", propslen); >> + result =3D -EINVAL; >> + } else { >> + const char **properties =3D malloc(sizeof(char*) * rh->num_props); >> + char *current =3D (rh->props); >> + int remaining =3D propslen; remaining starts positive >> + for (uint8_t i =3D 0; i < rh->num_props; i++) { >> + uint8_t proplen =3D strnlen(current, remaining); this ain't no uint8_t ;) so this takes a size_t (positive int is fine=20 accordingly) and returns one. a property can be longer than 255 chars=20 though, which makes the assignment overflow and gives us a=20 wrong/truncated proplen >> + if (proplen =3D=3D 0) { could trigger this if the overflow aligns right. >> + cfs_debug("property length 0"); >> + result =3D -EINVAL; >> + break; >> + } >> + if (proplen =3D=3D remaining) { can't be triggered by overflow since proplen must always be smaller than=20 remaining >> + cfs_debug("property not \\0 terminated"); >> + result =3D -EINVAL; >> + break; >> + } >> + if (current[0] < 'a' || current[0] > 'z') { this could be triggered depending on how exactly proplen overflows / the=20 property gets truncated >> + cfs_debug("property does not start with [a-z]"); >> + result =3D -EINVAL; >> + break; >> + } >> + properties[i] =3D current; >> + current[proplen] =3D '\0'; // ensure property is 0 terminated truncates an overly long property >> + remaining -=3D (proplen + 1); >> + current +=3D proplen + 1; and we start the next iteration at the truncated position >> + } >> + >> + if (remaining !=3D 0) { which likely makes us end up here (except if the msg was malformed with=20 not-null-terminated properties, in which case they might align again and=20 just their boundaries are wrong ;)) given that we assume remaining remains positive, and barring further=20 future bugs that invariant should hold (we start off positive, in each=20 iteration proplen must be < remaining to reach the subtraction which=20 therefore at most reduces remaining to 0), we could switch both=20 remaining and proplen to size_t ? or if the/a limit on property length is actually desired, we should=20 explicitly check for that (propslen <=3D limit * count at the start, and=20 then in the loop body still get a size_t from strnlen and check that the=20 result is < limit) >=20 >> + cfs_debug("leftover data after parsing %ul properties", rh->num_= props); >> + result =3D -EINVAL; >> + } >> + >> + if (result =3D=3D 0) { >> + cfs_debug("cfs_get_guest_config_properties: basic valid checked,= do request"); >> + result =3D cfs_create_guest_conf_properties_msg(outbuf, memdb, p= roperties, rh->num_props, rh->vmid); >> + } >> + >> + free(properties); >> + } >> } else if (request_id =3D=3D CFS_IPC_VERIFY_TOKEN) { >> =20 >> cfs_verify_token_request_header_t *rh =3D (cfs_verify_token_request_h= eader_t *) data; >=20 > [..] >=20