net: ipconfig: replace strncpy with strscpy_pad

[PATCH] net: ipconfig: replace strncpy with strscpy_pad

Posted by Pranav Tyagi 10 months ago

Replace the deprecated strncpy() function with strscpy_pad() as the
destination buffer is NUL-terminated and requires
trailing NUL-padding

Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
---
 net/ipv4/ipconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index c56b6fe6f0d7..7c238d19328f 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -1690,7 +1690,7 @@ static int __init ic_proto_name(char *name)
 			*v = 0;
 			if (kstrtou8(client_id, 0, dhcp_client_identifier))
 				pr_debug("DHCP: Invalid client identifier type\n");
-			strncpy(dhcp_client_identifier + 1, v + 1, 251);
+			strscpy_pad(dhcp_client_identifier + 1, v + 1, 251);
 			*v = ',';
 		}
 		return 1;
-- 
2.49.0

Re: [PATCH] net: ipconfig: replace strncpy with strscpy_pad

Posted by Nelson, Shannon 10 months ago

On 4/8/2025 11:57 AM, Pranav Tyagi wrote:
> 
> Replace the deprecated strncpy() function with strscpy_pad() as the
> destination buffer is NUL-terminated and requires
> trailing NUL-padding
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>

There should be a Fixes tag here, and usually we put the 'net' tree 
indicator inside the tag, like this: [PATCH net]


> ---
>   net/ipv4/ipconfig.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index c56b6fe6f0d7..7c238d19328f 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -1690,7 +1690,7 @@ static int __init ic_proto_name(char *name)
>                          *v = 0;
>                          if (kstrtou8(client_id, 0, dhcp_client_identifier))
>                                  pr_debug("DHCP: Invalid client identifier type\n");
> -                       strncpy(dhcp_client_identifier + 1, v + 1, 251);
> +                       strscpy_pad(dhcp_client_identifier + 1, v + 1, 251);

The strncpy() action, as well as the memcpy() into 
dhcp_client_identifier elsewhere, are not padding to the end, so I think 
this only needs to be null-terminated, not fully padded.  If full 
padding is needed, please let us know why.

sln

>                          *v = ',';
>                  }
>                  return 1;
> --
> 2.49.0
> 
>

Re: [PATCH] net: ipconfig: replace strncpy with strscpy_pad

Posted by Simon Horman 10 months ago

On Tue, Apr 08, 2025 at 02:44:42PM -0700, Nelson, Shannon wrote:
> On 4/8/2025 11:57 AM, Pranav Tyagi wrote:
> > 
> > Replace the deprecated strncpy() function with strscpy_pad() as the
> > destination buffer is NUL-terminated and requires
> > trailing NUL-padding
> > 
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> 
> There should be a Fixes tag here, and usually we put the 'net' tree
> indicator inside the tag, like this: [PATCH net]

FWIIW, this feels more line net-next material to me:
I'm not seeing a bug that is being fixed.

> 
> 
> > ---
> >   net/ipv4/ipconfig.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> > index c56b6fe6f0d7..7c238d19328f 100644
> > --- a/net/ipv4/ipconfig.c
> > +++ b/net/ipv4/ipconfig.c
> > @@ -1690,7 +1690,7 @@ static int __init ic_proto_name(char *name)
> >                          *v = 0;
> >                          if (kstrtou8(client_id, 0, dhcp_client_identifier))
> >                                  pr_debug("DHCP: Invalid client identifier type\n");
> > -                       strncpy(dhcp_client_identifier + 1, v + 1, 251);
> > +                       strscpy_pad(dhcp_client_identifier + 1, v + 1, 251);
> 
> The strncpy() action, as well as the memcpy() into dhcp_client_identifier
> elsewhere, are not padding to the end, so I think this only needs to be
> null-terminated, not fully padded.  If full padding is needed, please let
> us know why.
> 
> sln
> 
> >                          *v = ',';
> >                  }
> >                  return 1;
> > --
> > 2.49.0
> > 
> > 
>

Re: [PATCH] net: ipconfig: replace strncpy with strscpy_pad

Posted by Pranav Tyagi 10 months ago

On Wed, Apr 9, 2025 at 3:14 AM Nelson, Shannon <shannon.nelson@amd.com> wrote:
>
> On 4/8/2025 11:57 AM, Pranav Tyagi wrote:
> >
> > Replace the deprecated strncpy() function with strscpy_pad() as the
> > destination buffer is NUL-terminated and requires
> > trailing NUL-padding
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
>
> There should be a Fixes tag here, and usually we put the 'net' tree
> indicator inside the tag, like this: [PATCH net]
>
>
> > ---
> >   net/ipv4/ipconfig.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> > index c56b6fe6f0d7..7c238d19328f 100644
> > --- a/net/ipv4/ipconfig.c
> > +++ b/net/ipv4/ipconfig.c
> > @@ -1690,7 +1690,7 @@ static int __init ic_proto_name(char *name)
> >                          *v = 0;
> >                          if (kstrtou8(client_id, 0, dhcp_client_identifier))
> >                                  pr_debug("DHCP: Invalid client identifier type\n");
> > -                       strncpy(dhcp_client_identifier + 1, v + 1, 251);
> > +                       strscpy_pad(dhcp_client_identifier + 1, v + 1, 251);
>
> The strncpy() action, as well as the memcpy() into
> dhcp_client_identifier elsewhere, are not padding to the end, so I think
> this only needs to be null-terminated, not fully padded.  If full
> padding is needed, please let us know why.
>
> sln
>
> >                          *v = ',';
> >                  }
> >                  return 1;
> > --
> > 2.49.0
> >
> >
>

My initial assumption was on the fact that dhcp_client_identifier
is directly used in DHCP packet construction
and may be parsed byte-wise. But on going through the code again
I see that it does not require to be fully padded.
Would strscpy() suffice? as it ensures null-termination and
does not fully pad the buffer.

Regards
Pranav Tyagi

Re: [PATCH] net: ipconfig: replace strncpy with strscpy_pad

Posted by Nelson, Shannon 10 months ago

On 4/10/2025 12:59 PM, Pranav Tyagi wrote:
> 
> On Wed, Apr 9, 2025 at 3:14 AM Nelson, Shannon <shannon.nelson@amd.com> wrote:
>>
>> On 4/8/2025 11:57 AM, Pranav Tyagi wrote:
>>>
>>> Replace the deprecated strncpy() function with strscpy_pad() as the
>>> destination buffer is NUL-terminated and requires
>>> trailing NUL-padding
>>>
>>> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
>>
>> There should be a Fixes tag here, and usually we put the 'net' tree
>> indicator inside the tag, like this: [PATCH net]
>>
>>
>>> ---
>>>    net/ipv4/ipconfig.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
>>> index c56b6fe6f0d7..7c238d19328f 100644
>>> --- a/net/ipv4/ipconfig.c
>>> +++ b/net/ipv4/ipconfig.c
>>> @@ -1690,7 +1690,7 @@ static int __init ic_proto_name(char *name)
>>>                           *v = 0;
>>>                           if (kstrtou8(client_id, 0, dhcp_client_identifier))
>>>                                   pr_debug("DHCP: Invalid client identifier type\n");
>>> -                       strncpy(dhcp_client_identifier + 1, v + 1, 251);
>>> +                       strscpy_pad(dhcp_client_identifier + 1, v + 1, 251);
>>
>> The strncpy() action, as well as the memcpy() into
>> dhcp_client_identifier elsewhere, are not padding to the end, so I think
>> this only needs to be null-terminated, not fully padded.  If full
>> padding is needed, please let us know why.
>>
>> sln
>>
>>>                           *v = ',';
>>>                   }
>>>                   return 1;
>>> --
>>> 2.49.0
>>>
>>>
>>
> 
> My initial assumption was on the fact that dhcp_client_identifier
> is directly used in DHCP packet construction
> and may be parsed byte-wise. But on going through the code again
> I see that it does not require to be fully padded.
> Would strscpy() suffice? as it ensures null-termination and
> does not fully pad the buffer.

Yes, strscpy() should work.
sln


> 
> Regards
> Pranav Tyagi