proc: fix comm_write return value when truncated or error

[PATCH] proc: fix comm_write return value when truncated or error

Posted by Shengzhuo Wei 1 month, 3 weeks ago

When count exceeds TASK_COMM_LEN-1, comm_write() copies at most
TASK_COMM_LEN-1 bytes but returns the original count. This violates
write(2) semantics, which require returning the number of bytes
actually written.

The count parameter is size_t and should not be repurposed to carry a
negative error code on the same_thread_group() failure path.

Introduce a local len for the truncated length and a separate ssize_t
ret for the return value.

Fixes: 4614a696bd1c ("procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm")
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 fs/proc/base.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index d9acfa89c894bd1608580331e1d5b3018c59123b..5d34590dbe9d9f05147c3e6b34c615cbf0984b1c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1727,8 +1727,10 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
 	struct task_struct *p;
 	char buffer[TASK_COMM_LEN] = {};
 	const size_t maxlen = sizeof(buffer) - 1;
+	size_t len = count > maxlen ? maxlen : count;
+	ssize_t ret;
 
-	if (copy_from_user(buffer, buf, count > maxlen ? maxlen : count))
+	if (copy_from_user(buffer, buf, len))
 		return -EFAULT;
 
 	p = get_proc_task(inode);
@@ -1738,13 +1740,14 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
 	if (same_thread_group(current, p)) {
 		set_task_comm(p, buffer);
 		proc_comm_connector(p);
+		ret = len;
+	} else {
+		ret = -EINVAL;
 	}
-	else
-		count = -EINVAL;
 
 	put_task_struct(p);
 
-	return count;
+	return ret;
 }
 
 static int comm_show(struct seq_file *m, void *v)

---
base-commit: 2e68039281932e6dc37718a1ea7cbb8e2cda42e6
change-id: 20260424-fix_proc_write_return-cd48edb86600

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

Re: [PATCH] proc: fix comm_write return value when truncated or error

Posted by Andrew Morton 1 month, 3 weeks ago

On Fri, 24 Apr 2026 04:06:21 +0800 "Shengzhuo Wei" <me@cherr.cc> wrote:

> When count exceeds TASK_COMM_LEN-1, comm_write() copies at most
> TASK_COMM_LEN-1 bytes but returns the original count. This violates
> write(2) semantics, which require returning the number of bytes
> actually written.
> 
> The count parameter is size_t and should not be repurposed to carry a
> negative error code on the same_thread_group() failure path.
> 
> Introduce a local len for the truncated length and a separate ssize_t
> ret for the return value.

AI review asks a good question:
	https://sashiko.dev/#/patchset/20260424-fix_proc_write_return-v1-1-7a793c2aad32@cherr.cc

Re: [PATCH] proc: fix comm_write return value when truncated or error

Posted by Andrew Morton 1 month, 3 weeks ago

On Fri, 24 Apr 2026 04:06:21 +0800 "Shengzhuo Wei" <me@cherr.cc> wrote:

> When count exceeds TASK_COMM_LEN-1, comm_write() copies at most
> TASK_COMM_LEN-1 bytes but returns the original count. This violates
> write(2) semantics, which require returning the number of bytes
> actually written.
> 
> The count parameter is size_t and should not be repurposed to carry a
> negative error code on the same_thread_group() failure path.
> 
> Introduce a local len for the truncated length and a separate ssize_t
> ret for the return value.

Looks right to me.

> Fixes: 4614a696bd1c ("procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm")

December 2009.

Hopefully no userspace is depending on the broken return value of a
write to /proc/pid/comm.

Arguably we should leave the code as-is and add an apologetic comment
explaining the situation.

Re: [PATCH] proc: fix comm_write return value when truncated or error

Posted by Alexey Dobriyan 1 month, 3 weeks ago

On Fri, Apr 24, 2026 at 03:50:28AM -0700, Andrew Morton wrote:
> On Fri, 24 Apr 2026 04:06:21 +0800 "Shengzhuo Wei" <me@cherr.cc> wrote:
> 
> > When count exceeds TASK_COMM_LEN-1, comm_write() copies at most
> > TASK_COMM_LEN-1 bytes but returns the original count. This violates
> > write(2) semantics, which require returning the number of bytes
> > actually written.
> > 
> > The count parameter is size_t and should not be repurposed to carry a
> > negative error code on the same_thread_group() failure path.
> > 
> > Introduce a local len for the truncated length and a separate ssize_t
> > ret for the return value.
> 
> Looks right to me.

> > Fixes: 4614a696bd1c ("procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm")
> 
> December 2009.
> 
> Hopefully no userspace is depending on the broken return value of a
> write to /proc/pid/comm.
> 
> Arguably we should leave the code as-is and add an apologetic comment
> explaining the situation.

Yes, this issue must all over virtual filesystems.

Patch may break stuff in the other direction too:

if process is doing "full write" loop, and write hook gets string
which is too long, then the last piece will be written, not truncated
first part.

	/proc/alexey

Re: [PATCH] proc: fix comm_write return value when truncated or error

Posted by Shengzhuo Wei 1 month, 3 weeks ago

On 2026-04-24 16:28, Alexey Dobriyan wrote:
> > > Fixes: 4614a696bd1c ("procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm")
> > 
> > December 2009.
> > 
> > Hopefully no userspace is depending on the broken return value of a
> > write to /proc/pid/comm.
> > 
> > Arguably we should leave the code as-is and add an apologetic comment
> > explaining the situation.
> 

First, there's a plain bug regardless: assigning -EINVAL to a size_t count and
returning it as ssize_t produces a bogus positive value on the error path.
Introducing a dedicated ssize_t ret fixes this without changing any
success-path behavior, the change is on a permission failure path, so it's hard 
to imagine anyone depending on the broken behavior, but worth mentioning

> Yes, this issue must all over virtual filesystems.
> 
> Patch may break stuff in the other direction too:
> 
> if process is doing "full write" loop, and write hook gets string
> which is too long, then the last piece will be written, not truncated
> first part.

For the truncation case, I agree returning a short count breaks the
full-write idiom. But sliently truncating isnt't great either. Maybe we
should reject overlong writes and return -EINVAL instead? That's also an
ABI change and breaks the userspace, so it needs discussion, but at
least it fails loudly.

> 
> 	/proc/alexey

Very happy to hear from your opinions and sorry for my poor English.
Best regards.

Re: [PATCH] proc: fix comm_write return value when truncated or error

Posted by Alexey Dobriyan 1 month, 3 weeks ago

On Sat, Apr 25, 2026 at 02:03:41AM +0800, Shengzhuo Wei wrote:
> On 2026-04-24 16:28, Alexey Dobriyan wrote:
> > > > Fixes: 4614a696bd1c ("procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm")
> > > 
> > > December 2009.
> > > 
> > > Hopefully no userspace is depending on the broken return value of a
> > > write to /proc/pid/comm.
> > > 
> > > Arguably we should leave the code as-is and add an apologetic comment
> > > explaining the situation.
> > 
> 
> First, there's a plain bug regardless: assigning -EINVAL to a size_t count and
> returning it as ssize_t produces a bogus positive value on the error path.

There is no bug, write() will return -EINVAL as it should.

> Introducing a dedicated ssize_t ret fixes this without changing any
> success-path behavior, the change is on a permission failure path, so it's hard 
> to imagine anyone depending on the broken behavior, but worth mentioning