uiomove: Use size_t for the length argument
Needs ReviewPublic
Actions

Authored by des on Fri, Sep 4, 6:44 PM.

Details

Reviewers

kib
markj
andrew
manu

Summary

In many if not most cases, the caller has a size_t or off_t. Passing it
as an int may result in overflow.

PR: 298159

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76568
Build 73451: arc lint + arc unit

Event Timeline

des created this revision.Fri, Sep 4, 6:44 PM

Herald added a reviewer: andrew. · View Herald TranscriptFri, Sep 4, 6:44 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: riscv, ziaee, olce and 3 others. · View Herald Transcript

des requested review of this revision.Fri, Sep 4, 6:44 PM

Harbormaster completed remote builds in B76523: Diff 185853.Fri, Sep 4, 6:44 PM

des added a parent revision: D59412: vfs: Avoid integer overflow when faulting.Fri, Sep 4, 6:44 PM

Did you inspected all callers to ensure that they do not pass negative n/len and rely on n < 0 checks?

sys/amd64/amd64/uio_machdep.c
78–80	since you are changing the lines anyway
sys/powerpc/powerpc/uio_machdep.c
106	I think the change should add asserts that iov_len >= cnt, uio_resid >= cnt. and most importantly len >= cnt.

olce added inline comments.Fri, Sep 4, 9:50 PM

sys/amd64/amd64/uio_machdep.c
78–80	Please use `!= 0` in preference to `> 0` for unsigned values. (Same for occurrences below.)

BTW, it should be much easier to change the n/len argument to ssize_t. I think this is a way to go.

In D59413#1363320, @kib wrote:

BTW, it should be much easier to change the n/len argument to ssize_t. I think this is a way to go.

No, that would not fix the problem of callers passing a size_t to uiomove() without first checking for overflow.

In D59413#1363378, @des wrote:

In D59413#1363320, @kib wrote:

BTW, it should be much easier to change the n/len argument to ssize_t. I think this is a way to go.

No, that would not fix the problem of callers passing a size_t to uiomove() without first checking for overflow.

It would be only practical problem on 32bit kernels. And even then, if size_t overflows, the check for n < 0 catches it.

In D59413#1363429, @kib wrote:

It would be only practical problem on 32bit kernels. And even then, if size_t overflows, the check for n < 0 catches it.

Signed integer overflow is undefined behavior in C. I will not commit code that deliberately relies on it.

sys/powerpc/powerpc/uio_machdep.c
106	Here specifically or across all targets?

In D59413#1363431, @des wrote:

In D59413#1363429, @kib wrote:

It would be only practical problem on 32bit kernels. And even then, if size_t overflows, the check for n < 0 catches it.

Signed integer overflow is undefined behavior in C. I will not commit code that deliberately relies on it.

It is not, in kernel. We compile with -fwrap specifically to have this defined.

sys/powerpc/powerpc/uio_machdep.c
106	Across all targets, I did not wanted to copy/paste the same obvious suggestion.

des added inline comments.Sat, Sep 5, 3:56 PM

sys/powerpc/powerpc/uio_machdep.c
106	These assertions seem redundant to me given that we start out with `cnt = iov->iov_len` and then check that `cnt <= len` and shorten it if it isn't. The only thing we're missing is a check that `len` does not exceed `iov->iov_resid`.

Harbormaster completed remote builds in B76568: Diff 185933.Sat, Sep 5, 3:58 PM

des marked 3 inline comments as done.Sat, Sep 5, 4:10 PM

In D59413#1363464, @kib wrote:

In D59413#1363431, @des wrote:

In D59413#1363429, @kib wrote:

It would be only practical problem on 32bit kernels. And even then, if size_t overflows, the check for n < 0 catches it.

Signed integer overflow is undefined behavior in C. I will not commit code that deliberately relies on it.

It is not, in kernel. We compile with -fwrap specifically to have this defined.

There's thousands of places where this is done, iirc the conversations when we added the flag.

So if you still insist on using size_t for length, I have to ask again, were all callers inspected to make sure that they do not rely on the behavior for len < 0 'do nothing'?

In D59413#1363746, @kib wrote:

So if you still insist on using size_t for length, I have to ask again, were all callers inspected to make sure that they do not rely on the behavior for len < 0 'do nothing'?

I'm working on it. There are over 200, so it's going to take some time. I'll add those patches to D59412.

kern/subr_uio.c:355 still says uiomove() accepts a signed argument.

In D59413#1363786, @des wrote:

In D59413#1363746, @kib wrote:

So if you still insist on using size_t for length, I have to ask again, were all callers inspected to make sure that they do not rely on the behavior for len < 0 'do nothing'?

I'm working on it. There are over 200, so it's going to take some time. I'll add those patches to D59412.

Claude claimed 352 calls.

Also, claude generated code whose exit path leaked buffer locks, etc, so adding that to your thinking would be good, if you weren't already considering it :)

rebase

Harbormaster completed remote builds in B76641: Diff 186146.Mon, Sep 7, 5:09 PM