linux-kernel - Re: [PATCH v3] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for keyfile format

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <C740F1C4-CA2A-425B-8E60-0EF5C2C15270@redhat.com>
Date: Mon, 18 Mar 2024 22:42:52 +0530
From: Ani Sinha <anisinha@...hat.com>
To: Easwar Hariharan <eahariha@...ux.microsoft.com>
Cc: Shradha Gupta <shradhagupta@...ux.microsoft.com>,
 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>,
 Olaf Hering <olaf@...fle.de>,
 Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v3] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format



> On 18 Mar 2024, at 21:49, Easwar Hariharan <eahariha@...ux.microsoft.com> wrote:
> 
> On 3/17/2024 7:45 PM, Shradha Gupta wrote:
>> If the network configuration strings are passed as a combination of IPv and
> 
> Repeating a few unaddressed comments from v2.
> 
> Missing a 4 in the IPv4 string here
> 
>> IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> 
> You probably want to add a space so it reads as *...KVP daemon does not*, or contract it to *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.
>> 
>> Testcases ran:Rhel 9, Hyper-V VMs
>>              (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
>> Signed-off-by: Shradha Gupta <shradhagupta@...ux.microsoft.com>
>> ---
>> Changes in v3
>> * Introduced a macro for the output string size
>> * Added cound checks and used strncpy instead of strncpy
>> * Rearranged code to reduce total lines of code
>> ---
>> tools/hv/hv_kvp_daemon.c | 177 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 136 insertions(+), 41 deletions(-)
>> 
>> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
>> index 318e2dad27e0..156cef99d361 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,11 @@ static struct utsname uts_buf;
>> 
>> #define MAX_FILE_NAME 100
>> #define ENTRIES_PER_BLOCK 50
>> +/*
>> + * Change this entry if the number of addresses increases in future
>> + */
>> +#define MAX_IP_ENTRIES 64
>> +#define OUTSTR_BUF_SIZE ((INET6_ADDRSTRLEN + 1) * MAX_IP_ENTRIES)
>> 
>> struct kvp_record {
>> char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
>> @@ -1171,6 +1182,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;
>> +
>> + return -EINVAL;
>> +}
>> +
>> /*
>>  * Only IPv4 subnet strings needs to be converted to plen
>>  * For IPv6 the subnet is already privided in plen format
>> @@ -1197,14 +1220,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;
>> +
>> + memset(addr, 0, sizeof(addr));
> 
> Echoing Ani, you don't need this memset here since your first step in the loop below is to
> memset(addr, 0).
> 
>> +
>> + if (type == DNS)
>> + param_name = "dns";
>> + else if (type == GATEWAY)
>> + param_name = "gateway";
>> + else
>> + return -EINVAL;
>> +
>> + output_str = (char *)calloc(OUTSTR_BUF_SIZE, sizeof(char));
>> + if (!output_str)
>> + return -ENOMEM;
>> +
>> + while (1) {
>> + memset(addr, 0, sizeof(addr));
>> +
>> + if (!parse_ip_val_buffer(ip_string, &ip_offset, addr,
>> +  (MAX_IP_ADDR_SIZE * 2)))
>> + break;
>> +
>> + ip_ver = ip_version_check(addr);
>> + if (ip_ver < 0)
>> + continue;
>> +
>> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
>> +     (ip_ver == IPV6 && ip_sec == IPV6)) {
>> + /*
>> +  * do a bound check to avoid out-of bound writes
>> +  */
>> + if ((OUTSTR_BUF_SIZE - strlen(output_str)) >
>> +     (strlen(addr) + 1)) {
>> + strncat(output_str, addr,
>> + OUTSTR_BUF_SIZE - strlen(output_str));
>> + strncat(output_str, ",",
>> + OUTSTR_BUF_SIZE - strlen(output_str));
>> + }
>> + } else {
>> + continue;
>> + }
>> + }
>> +
>> + if (strlen(output_str)) {
>> + output_str[strlen(output_str) - 1] = '\0';
> 
> You don't need this since you're using strncat which adds its own '\0'.

If I understand this correctly, this code simply eliminates the extra “,” character in the end. Therefore it is needed.
Since it is not obvious, in the previous review and before, I asked the author to add a comment to explain this clearly.

> I wasn't quite able to follow along 
> on the discussion between Ani and you, so putting this in here in case it wasn't already mentioned.
> 
>> + error = fprintf(f, "%s=%s\n", param_name, output_str);
>> + }
>> +
>> + free(output_str);
>> + return error;
>> +}
> 
> <snip>