From: Mykyta Yatsenko <yatsenko@meta.com>

strncpy_from_user_nofault should return the length of the copied string
including the trailing NUL, but if the argument unsafe_addr points to
an empty string ({'\0'}), the return value is 0.

This happens as strncpy_from_user copies terminal symbol into dst
and returns 0 (as expected), but strncpy_from_user_nofault does not
modify ret as it is not equal to count and not greater than 0, so 0 is
returned, which contradicts the contract.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 mm/maccess.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index 8f0906180a94..831b4dd7296c 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -196,7 +196,7 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
 	if (ret >= count) {
 		ret = count;
 		dst[ret - 1] = '\0';
-	} else if (ret > 0) {
+	} else if (ret >= 0) {
 		ret++;
 	}
 
-- 
2.49.0

On Tue, 22 Apr 2025 14:14:49 +0100 Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote:

> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> strncpy_from_user_nofault should return the length of the copied string
> including the trailing NUL, but if the argument unsafe_addr points to
> an empty string ({'\0'}), the return value is 0.
> 
> This happens as strncpy_from_user copies terminal symbol into dst
> and returns 0 (as expected), but strncpy_from_user_nofault does not
> modify ret as it is not equal to count and not greater than 0, so 0 is
> returned, which contradicts the contract.
> 
> ...
>

Thanks.

Does this fix any known runtime issue?  If so, please fully describe this?

> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -196,7 +196,7 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>  	if (ret >= count) {
>  		ret = count;
>  		dst[ret - 1] = '\0';
> -	} else if (ret > 0) {
> +	} else if (ret >= 0) {
>  		ret++;
>  	}
>

On 4/23/25 01:20, Andrew Morton wrote:
> On Tue, 22 Apr 2025 14:14:49 +0100 Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote:
>
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> strncpy_from_user_nofault should return the length of the copied string
>> including the trailing NUL, but if the argument unsafe_addr points to
>> an empty string ({'\0'}), the return value is 0.
>>
>> This happens as strncpy_from_user copies terminal symbol into dst
>> and returns 0 (as expected), but strncpy_from_user_nofault does not
>> modify ret as it is not equal to count and not greater than 0, so 0 is
>> returned, which contradicts the contract.
>>
>> ...
>>
> Thanks.
>
> Does this fix any known runtime issue?  If so, please fully describe this?
Not that I'm aware of. The issue could be found when trying to copy empty
user space string in BPF program (and relying on return value).There are 
some usage of
`strncpy_from_user_nofault` in tracing subsystem, but I'm not sure how to
hit those code paths.
>
>> --- a/mm/maccess.c
>> +++ b/mm/maccess.c
>> @@ -196,7 +196,7 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>>   	if (ret >= count) {
>>   		ret = count;
>>   		dst[ret - 1] = '\0';
>> -	} else if (ret > 0) {
>> +	} else if (ret >= 0) {
>>   		ret++;
>>   	}
>>

On Wed, 23 Apr 2025 12:37:46 +0100
Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote:

> > Does this fix any known runtime issue?  If so, please fully describe this?  
> Not that I'm aware of. The issue could be found when trying to copy empty
> user space string in BPF program (and relying on return value).There are 
> some usage of
> `strncpy_from_user_nofault` in tracing subsystem, but I'm not sure how to
> hit those code paths.
> >  

Although your patch found a bug in the tracing subsystem, this wasn't the
cause. It only cared if the read faulted or not. It was incorrectly
checking for zero as non fault when in reality, it needed to check >= 0.

With that fixed, it should work the same with or without this patch.

-- Steve

On 4/23/25 14:59, Steven Rostedt wrote:
> On Wed, 23 Apr 2025 12:37:46 +0100
> Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote:
>
>>> Does this fix any known runtime issue?  If so, please fully describe this?
>> Not that I'm aware of. The issue could be found when trying to copy empty
>> user space string in BPF program (and relying on return value).There are
>> some usage of
>> `strncpy_from_user_nofault` in tracing subsystem, but I'm not sure how to
>> hit those code paths.
>>>   
> Although your patch found a bug in the tracing subsystem, this wasn't the
> cause. It only cared if the read faulted or not. It was incorrectly
> checking for zero as non fault when in reality, it needed to check >= 0.
>
> With that fixed, it should work the same with or without this patch.
>
> -- Steve
Sure, I had in mind usages from trace_probe_kernel.h,
namely fetch_store_string_user, fetch_store_string, having a second look,
it appears these only used in trace_events_synth.c, and we are good there.

On Tue, Apr 22, 2025 at 6:15 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> From: Mykyta Yatsenko <yatsenko@meta.com>
>
> strncpy_from_user_nofault should return the length of the copied string
> including the trailing NUL, but if the argument unsafe_addr points to
> an empty string ({'\0'}), the return value is 0.
>
> This happens as strncpy_from_user copies terminal symbol into dst
> and returns 0 (as expected), but strncpy_from_user_nofault does not
> modify ret as it is not equal to count and not greater than 0, so 0 is
> returned, which contradicts the contract.
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>  mm/maccess.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/mm/maccess.c b/mm/maccess.c
> index 8f0906180a94..831b4dd7296c 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -196,7 +196,7 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>         if (ret >= count) {
>                 ret = count;
>                 dst[ret - 1] = '\0';
> -       } else if (ret > 0) {
> +       } else if (ret >= 0) {
>                 ret++;
>         }
>
> --
> 2.49.0
>