um: proc/exitcode: fix simple_strtol() out-of-bounds read

[PATCH] um: proc/exitcode: fix simple_strtol() out-of-bounds read

Posted by Shengzhuo Wei 1 month, 3 weeks ago

The stack buffer 'buf' is declared as char[sizeof("nnnnn\0")] (7 bytes)
and the copy size is min(count, sizeof(buf)).  When a user writes 7 or
more bytes, copy_from_user fills all 7 bytes without a NUL terminator.
The subsequent call to simple_strtol() expects a NUL-terminated string
and will read past the end of buf on the stack.

write(2) should report the number of bytes consumed. Returning the original
count would claim success even when the input was truncated, so userspace
cannot detect it.

Clamp the copy length to sizeof(buf)-1, add a terminator, and return the
consumed length.

Fixes: 201f99f170df ("uml: check length in exitcode_proc_write()")
Fixes: e16f5350d4cf ("uml: get declaration of simple_strtoul")
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 arch/um/kernel/exitcode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/um/kernel/exitcode.c b/arch/um/kernel/exitcode.c
index 43edc2aa57e4fbd4a3d24f96878c76f9f8fd4eaa..8de404ff21a213918c5351bc20a6e047bf1b93f5 100644
--- a/arch/um/kernel/exitcode.c
+++ b/arch/um/kernel/exitcode.c
@@ -43,16 +43,17 @@ static ssize_t exitcode_proc_write(struct file *file,
 	size_t size;
 	int tmp;
 
-	size = min(count, sizeof(buf));
+	size = min(count, sizeof(buf) - 1);
 	if (copy_from_user(buf, buffer, size))
 		return -EFAULT;
+	buf[size] = '\0';
 
 	tmp = simple_strtol(buf, &end, 0);
 	if ((*end != '\0') && !isspace(*end))
 		return -EINVAL;
 
 	uml_exitcode = tmp;
-	return count;
+	return size;
 }
 
 static const struct proc_ops exitcode_proc_ops = {

---
base-commit: 6596a02b207886e9e00bb0161c7fd59fea53c081
change-id: 20260423-fix_exitcode-908061ece624

Best regards,
-- 
Shengzhuo Wei <me@cherr.cc>

Re: [PATCH] um: proc/exitcode: fix simple_strtol() out-of-bounds read

Posted by Shengzhuo Wei 1 month, 2 weeks ago

On 2026-04-23 01:39, Shengzhuo Wei wrote:
> write(2) should report the number of bytes consumed. Returning the original
> count would claim success even when the input was truncated, so userspace
> cannot detect it.
> 
> Clamp the copy length to sizeof(buf)-1, add a terminator, and return the
> consumed length.
> 
> Fixes: 201f99f170df ("uml: check length in exitcode_proc_write()")
> Fixes: e16f5350d4cf ("uml: get declaration of simple_strtoul")
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
>  arch/um/kernel/exitcode.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/kernel/exitcode.c b/arch/um/kernel/exitcode.c
> index 43edc2aa57e4fbd4a3d24f96878c76f9f8fd4eaa..8de404ff21a213918c5351bc20a6e047bf1b93f5 100644
> --- a/arch/um/kernel/exitcode.c
> +++ b/arch/um/kernel/exitcode.c
> @@ -43,16 +43,17 @@ static ssize_t exitcode_proc_write(struct file *file,
>  	size_t size;
>  	int tmp;
>  
> -	size = min(count, sizeof(buf));
> +	size = min(count, sizeof(buf) - 1);
>  	if (copy_from_user(buf, buffer, size))
>  		return -EFAULT;
> +	buf[size] = '\0';
>  
>  	tmp = simple_strtol(buf, &end, 0);
>  	if ((*end != '\0') && !isspace(*end))
>  		return -EINVAL;
>  
>  	uml_exitcode = tmp;
> -	return count;
> +	return size;

There is another problem that have been talked about in another patch.
Not consuming the whole string passed by user may (and will) break
some programs that want to ensure "FULL WRITE" that use some function
like "while (len > 0) { len -= write(); } ".

Should we just let the return count as-is and fix the OOB problem only?

Link: https://lore.kernel.org/all/20260424063531.d1508a3c79c4b7808ed04420@linux-foundation.org/t/#u
Link: https://lore.kernel.org/all/ef2301c4-bbc5-496e-b9d2-9adcff8b8d42@p183/

Regards,
Shengzhuo Wei

>  }
>  
>  static const struct proc_ops exitcode_proc_ops = {
> 
> ---

Re: [PATCH] um: proc/exitcode: fix simple_strtol() out-of-bounds read

Posted by David Laight 1 month, 3 weeks ago

On Thu, 23 Apr 2026 01:39:25 +0800
"Shengzhuo Wei" <me@cherr.cc> wrote:

> The stack buffer 'buf' is declared as char[sizeof("nnnnn\0")] (7 bytes)
> and the copy size is min(count, sizeof(buf)).  When a user writes 7 or
> more bytes, copy_from_user fills all 7 bytes without a NUL terminator.
> The subsequent call to simple_strtol() expects a NUL-terminated string
> and will read past the end of buf on the stack.

You should probably also mention that write(, "123", 3) will lead to
buf[3] being read - which is uninitialised stack.

	David

> 
> write(2) should report the number of bytes consumed. Returning the original
> count would claim success even when the input was truncated, so userspace
> cannot detect it.
> 
> Clamp the copy length to sizeof(buf)-1, add a terminator, and return the
> consumed length.
> 
> Fixes: 201f99f170df ("uml: check length in exitcode_proc_write()")
> Fixes: e16f5350d4cf ("uml: get declaration of simple_strtoul")
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
>  arch/um/kernel/exitcode.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/kernel/exitcode.c b/arch/um/kernel/exitcode.c
> index 43edc2aa57e4fbd4a3d24f96878c76f9f8fd4eaa..8de404ff21a213918c5351bc20a6e047bf1b93f5 100644
> --- a/arch/um/kernel/exitcode.c
> +++ b/arch/um/kernel/exitcode.c
> @@ -43,16 +43,17 @@ static ssize_t exitcode_proc_write(struct file *file,
>  	size_t size;
>  	int tmp;
>  
> -	size = min(count, sizeof(buf));
> +	size = min(count, sizeof(buf) - 1);
>  	if (copy_from_user(buf, buffer, size))
>  		return -EFAULT;
> +	buf[size] = '\0';
>  
>  	tmp = simple_strtol(buf, &end, 0);
>  	if ((*end != '\0') && !isspace(*end))
>  		return -EINVAL;
>  
>  	uml_exitcode = tmp;
> -	return count;
> +	return size;
>  }
>  
>  static const struct proc_ops exitcode_proc_ops = {
> 
> ---
> base-commit: 6596a02b207886e9e00bb0161c7fd59fea53c081
> change-id: 20260423-fix_exitcode-908061ece624
> 
> Best regards,

Re: [PATCH] um: proc/exitcode: fix simple_strtol() out-of-bounds read

Posted by Shengzhuo Wei 1 month, 3 weeks ago

On 2026-04-22 21:45, David Laight wrote:
> On Thu, 23 Apr 2026 01:39:25 +0800
> "Shengzhuo Wei" <me@cherr.cc> wrote:
> 
> > The stack buffer 'buf' is declared as char[sizeof("nnnnn\0")] (7 bytes)
> > and the copy size is min(count, sizeof(buf)).  When a user writes 7 or
> > more bytes, copy_from_user fills all 7 bytes without a NUL terminator.
> > The subsequent call to simple_strtol() expects a NUL-terminated string
> > and will read past the end of buf on the stack.
> 
> You should probably also mention that write(, "123", 3) will lead to
> buf[3] being read - which is uninitialised stack.
> 
> 	David

Thanks for the review, will fix in v2.

Best regards