| 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: <20240315140914.GA14685@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Fri, 15 Mar 2024 07:09:14 -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 10:22:12PM -0700, Shradha Gupta wrote:
> 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
Is this needed with the bound check above. I am trying to keep parity with the rest of the
file.
> >
> > > + }
> > > + 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