| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240313052212.GB22465@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Tue, 12 Mar 2024 22:22:12 -0700
From: Shradha Gupta <shradhagupta@...ux.microsoft.com>
To: Easwar Hariharan <eahariha@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
"K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
Long Li <longli@...rosoft.com>,
Michael Kelley <mikelley@...rosoft.com>,
Olaf Hering <olaf@...fle.de>, Ani Sinha <anisinha@...hat.com>,
Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination
for keyfile format
On Tue, Mar 12, 2024 at 09:58:03AM -0700, Easwar Hariharan wrote:
> On 3/12/2024 5:38 AM, Shradha Gupta wrote:
> > If the network configuration strings are passed as a combination of IPv and
>
> *IPv4*
>
> > IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> *does not/doesn't*
> > 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)
>
> As mentioned by Jakub[1], what value does this information provide?
> Please follow Haiyang's suggestion [2] and put SKU and test information, or just
> skip it.
>
> [1] https://lore.kernel.org/all/20240307072923.6cc8a2ba@kernel.org/
> [2] https://lore.kernel.org/all/DM6PR21MB14817597567C638DEF020FE3CA202@DM6PR21MB1481.namprd21.prod.outlook.com/
Hi Easwar, unlike the other patch this one has details about the tests that were performed.
Since this is Hyper-v VMs specific, I could not add details around SKU or LISA tests(as it
could not be tested using LISA). In the last patch we had missed the IPv4, IPv6 combination
testing(which had some design issues). That's why I feel it is important to call it out in
this patch.
>
> > Signed-off-by: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> > ---
> > 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
>
> Is this a limitation defined by hv_kvp? If so, is it possible it may change in a later
> version? A comment would help here
Sure, would update accordingly
>
> >
> > 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;
>
> You can skip the else here...
>
> > + else
> > + return -EINVAL;
>
> ...and you can skip the else here as well and just return -EINVAL
right, will change this in the next version.
>
> > +}
> > +
> > /*
> > * 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));
> > +
> > + 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;
> > + }
> > +
> > + 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, ",");
>
> Prefer strncat() here
>
> > + }
> > + memset(addr, 0, sizeof(addr));
> > +
> > + } else {
> > + memset(addr, 0, sizeof(addr));
> > + continue;
> > + }
> > + }
> > +
> > + if (strlen(output_str)) {
> > + output_str[strlen(output_str) - 1] = '\0';
> > + error = fprintf(f, "%s=%s\n", param_name, output_str);
> > + if (error < 0)
> > + goto cleanup;
> > + }
> > +
> > +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;
>
> nit: Can we keep ip_ver through all the functions for consistency
sure.
>
> <snip>
>
> Thanks,
> Easwar
Powered by blists - more mailing lists