On Tue, Mar 12, 2024 at 08:09:03PM +0530, Ani Sinha wrote: > > > On Tue, 12 Mar 2024, Shradha Gupta wrote: > > > If the network configuration strings are passed as a combination of IPv and > > IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile > > configuration format. > > With these changes, the keyfile config generation logic scans through the > > list twice to generate IPv4 and IPv6 sections for the configuration files > > to handle this support. > > > > Built-on: Rhel9 > > Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination) > > Signed-off-by: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx> > > --- > > Changes in v2 > > * Use calloc to avoid initialization later > > * Return standard error codes > > * Free the output_str pointer on completion > > * Add out-of bound checks while writing to buffers > > --- > > tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++---------- > > 1 file changed, 132 insertions(+), 41 deletions(-) > > > > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c > > index 318e2dad27e0..ae65be004eb1 100644 > > --- a/tools/hv/hv_kvp_daemon.c > > +++ b/tools/hv/hv_kvp_daemon.c > > @@ -76,6 +76,12 @@ enum { > > DNS > > }; > > > > +enum { > > + IPV4 = 1, > > + IPV6, > > + IP_TYPE_MAX > > +}; > > + > > static int in_hand_shake; > > > > static char *os_name = ""; > > @@ -102,6 +108,7 @@ static struct utsname uts_buf; > > > > #define MAX_FILE_NAME 100 > > #define ENTRIES_PER_BLOCK 50 > > +#define MAX_IP_ENTRIES 64 > > > > struct kvp_record { > > char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE]; > > @@ -1171,6 +1178,18 @@ static int process_ip_string(FILE *f, char *ip_string, int type) > > return 0; > > } > > > > +int ip_version_check(const char *input_addr) > > +{ > > + struct in6_addr addr; > > + > > + if (inet_pton(AF_INET, input_addr, &addr)) > > + return IPV4; > > + else if (inet_pton(AF_INET6, input_addr, &addr)) > > + return IPV6; > > + else > > + return -EINVAL; > > +} > > + > > /* > > * Only IPv4 subnet strings needs to be converted to plen > > * For IPv6 the subnet is already privided in plen format > > @@ -1197,14 +1216,71 @@ static int kvp_subnet_to_plen(char *subnet_addr_str) > > return plen; > > } > > > > +static int process_dns_gateway_nm(FILE *f, char *ip_string, int type, > > + int ip_sec) > > +{ > > + char addr[INET6_ADDRSTRLEN], *output_str; > > + int ip_offset = 0, error = 0, ip_ver; > > + char *param_name; > > + > > + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES, > > + sizeof(char)); > > Can we define INET6_ADDRSTRLEN * MAX_IP_ENTRIES as something like > OUTSTR_BUF_SZ or some such? Then it becomes more readable here and below. sure, that makes sense. > > > + > > + if (!output_str) > > + return -ENOMEM; > > + > > + memset(addr, 0, sizeof(addr)); > > > > + > > + if (type == DNS) { > > + param_name = "dns"; > > + } else if (type == GATEWAY) { > > + param_name = "gateway"; > > + } else { > > + error = -EINVAL; > > + goto cleanup; > > + } > > If you move the above check before you allocate memory for output_str, you > can return right away without doing a free(). Right, I'll do that > > > + > > + while (parse_ip_val_buffer(ip_string, &ip_offset, addr, > > + (MAX_IP_ADDR_SIZE * 2))) { > > + ip_ver = ip_version_check(addr); > > + if (ip_ver < 0) > > + continue; > > + > > + if ((ip_ver == IPV4 && ip_sec == IPV4) || > > + (ip_ver == IPV6 && ip_sec == IPV6)) { > > + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) > > > + (strlen(addr))) { > > + strcat(output_str, addr); > > + strcat(output_str, ","); > > Your bound check does not take into consideration one additional character > (the ","). It should be > > (INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str) > strlen(addr) + 1 > > > + } > > + memset(addr, 0, sizeof(addr)); > > + > > + } else { > > + memset(addr, 0, sizeof(addr)); > > if you do memset() at the beginning of the loop, you do not need to do > this separately for both branches. Plus there would be no need to do this > at the beginning of the function as well. > So you could do something like: > > while(1) { > memset(addr ...); > if (!parse_ip_val_buffer(...)) > break; > ... > } makes sense. > > > > + continue; > > + } > > + } > > + > > + if (strlen(output_str)) { > > // remove the last comma character > > > + output_str[strlen(output_str) - 1] = '\0'; > > + error = fprintf(f, "%s=%s\n", param_name, output_str); > > + if (error < 0) > > + goto cleanup; > > You need to free memory regardless of whether there is an error or not. Right, no need for the above check. Thanks > > > + } > > + > > +cleanup: > > + free(output_str); > > + return error; > > +} > > + > > static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet, > > - int is_ipv6) > > + int ip_sec) > > { > > char addr[INET6_ADDRSTRLEN]; > > char subnet_addr[INET6_ADDRSTRLEN]; > > int error, i = 0; > > int ip_offset = 0, subnet_offset = 0; > > - int plen; > > + int plen, ip_ver; > > > > memset(addr, 0, sizeof(addr)); > > memset(subnet_addr, 0, sizeof(subnet_addr)); > > @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet, > > subnet_addr, > > (MAX_IP_ADDR_SIZE * > > 2))) { > > - if (!is_ipv6) > > + ip_ver = ip_version_check(addr); > > + if (ip_ver < 0) > > + continue; > > + > > + if (ip_ver == IPV4 && ip_sec == IPV4) > > plen = kvp_subnet_to_plen((char *)subnet_addr); > > - else > > + else if (ip_ver == IPV6 && ip_sec == IPV6) > > plen = atoi(subnet_addr); > > + else > > + continue; > > > > if (plen < 0) > > return plen; > > @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet, > > > > static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val) > > { > > - int error = 0; > > + int error = 0, ip_type; > > char if_filename[PATH_MAX]; > > char nm_filename[PATH_MAX]; > > FILE *ifcfg_file, *nmfile; > > char cmd[PATH_MAX]; > > - int is_ipv6 = 0; > > char *mac_addr; > > int str_len; > > > > @@ -1421,52 +1502,62 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val) > > if (error) > > goto setval_error; > > > > - if (new_val->addr_family & ADDR_FAMILY_IPV6) { > > - error = fprintf(nmfile, "\n[ipv6]\n"); > > - if (error < 0) > > - goto setval_error; > > - is_ipv6 = 1; > > - } else { > > - error = fprintf(nmfile, "\n[ipv4]\n"); > > - if (error < 0) > > - goto setval_error; > > - } > > - > > /* > > - * Now we populate the keyfile format > > + * The keyfile format expects the IPv6 and IPv4 configuration in > > + * different sections. Therefore we iterate through the list twice, > > + * once to populate the IPv4 section and the next time for IPv6 > > */ > > + ip_type = IPV4; > > + do { > > + if (ip_type == IPV4) { > > + error = fprintf(nmfile, "\n[ipv4]\n"); > > + if (error < 0) > > + goto setval_error; > > + } else { > > + error = fprintf(nmfile, "\n[ipv6]\n"); > > + if (error < 0) > > + goto setval_error; > > + } > > > > - if (new_val->dhcp_enabled) { > > - error = kvp_write_file(nmfile, "method", "", "auto"); > > - if (error < 0) > > - goto setval_error; > > - } else { > > - error = kvp_write_file(nmfile, "method", "", "manual"); > > + /* > > + * Now we populate the keyfile format > > + */ > > + > > + if (new_val->dhcp_enabled) { > > + error = kvp_write_file(nmfile, "method", "", "auto"); > > + if (error < 0) > > + goto setval_error; > > + } else { > > + error = kvp_write_file(nmfile, "method", "", "manual"); > > + if (error < 0) > > + goto setval_error; > > + } > > + > > + /* > > + * Write the configuration for ipaddress, netmask, gateway and > > + * name services > > + */ > > + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr, > > + (char *)new_val->sub_net, > > + ip_type); > > if (error < 0) > > goto setval_error; > > - } > > > > - /* > > - * Write the configuration for ipaddress, netmask, gateway and > > - * name services > > - */ > > - error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr, > > - (char *)new_val->sub_net, is_ipv6); > > - if (error < 0) > > - goto setval_error; > > - > > - /* we do not want ipv4 addresses in ipv6 section and vice versa */ > > - if (is_ipv6 != is_ipv4((char *)new_val->gate_way)) { > > - error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way); > > + error = process_dns_gateway_nm(nmfile, > > + (char *)new_val->gate_way, > > + GATEWAY, ip_type); > > if (error < 0) > > goto setval_error; > > - } > > > > - if (is_ipv6 != is_ipv4((char *)new_val->dns_addr)) { > > - error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr); > > + error = process_dns_gateway_nm(nmfile, > > + (char *)new_val->dns_addr, DNS, > > + ip_type); > > if (error < 0) > > goto setval_error; > > - } > > + > > + ip_type++; > > + } while (ip_type < IP_TYPE_MAX); > > + > > fclose(nmfile); > > fclose(ifcfg_file); > > > > -- > > 2.34.1 > > > >