[PATCH] libfs: fix accidental overflow in offset calculation

[PATCH] libfs: fix accidental overflow in offset calculation

[Justin Stitt]

Running syzkaller with the newly reintroduced signed integer overflow
sanitizer gives this report:

[ 6008.464680] UBSAN: signed-integer-overflow in ../fs/libfs.c:149:11
[ 6008.468664] 9223372036854775807 + 16387 cannot be represented in type 'loff_t' (aka 'long long')
[ 6008.474167] CPU: 1 PID: 1214 Comm: syz-executor.0 Not tainted 6.8.0-rc2-00041-gec7cb1052e44-dirty #15
[ 6008.479662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 6008.485276] Call Trace:
[ 6008.486819]  <TASK>
[ 6008.488258]  dump_stack_lvl+0x93/0xd0
[ 6008.490535]  handle_overflow+0x171/0x1b0
[ 6008.492957]  dcache_dir_lseek+0x3bf/0x3d0
...

Use the check_add_overflow() helper to gracefully check for
unintentional overflow causing wraparound in our offset calculations.

Link: https://github.com/llvm/llvm-project/pull/82432 [1]
Closes: https://github.com/KSPP/linux/issues/359
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---

Historically, the signed integer overflow sanitizer did not work in the
kernel due to its interaction with `-fwrapv` but this has since been
changed [1] in the newest version of Clang. It was re-enabled in the
kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
sanitizer").

Here's the syzkaller reproducer:

| # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox:
| # SandboxArg:0 Leak:false NetInjection:false NetDevices:false
| # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false
| # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false
| # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false
| # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false
| # Fault:false FaultCall:0 FaultNth:0}}
| r0 = openat$sysfs(0xffffffffffffff9c, &(0x7f0000000000)='/sys/kernel/tracing', 0x0, 0x0)
| lseek(r0, 0x4003, 0x0)
| lseek(r0, 0x7fffffffffffffff, 0x1)

... which was used against Kees' tree here (v6.8rc2):
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer

... with this config:
https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4
---
 fs/libfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 3a6f2cb364f8..3fdc1aaddd45 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
 	struct dentry *dentry = file->f_path.dentry;
 	switch (whence) {
 		case 1:
-			offset += file->f_pos;
+			/* cannot represent offset with loff_t */
+			if (check_add_overflow(offset, file->f_pos, &offset))
+				return -EOVERFLOW;
 			fallthrough;
 		case 0:
 			if (offset >= 0)
@@ -422,7 +424,9 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
 {
 	switch (whence) {
 	case SEEK_CUR:
-		offset += file->f_pos;
+		/* cannot represent offset with loff_t */
+		if (check_add_overflow(offset, file->f_pos, &offset))
+			return -EOVERFLOW;
 		fallthrough;
 	case SEEK_SET:
 		if (offset >= 0)

---
base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc
change-id: 20240509-b4-sio-libfs-67947244a4a3

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Al Viro]

On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote:
> @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
>  	struct dentry *dentry = file->f_path.dentry;
>  	switch (whence) {
>  		case 1:
> -			offset += file->f_pos;
> +			/* cannot represent offset with loff_t */
> +			if (check_add_overflow(offset, file->f_pos, &offset))
> +				return -EOVERFLOW;

Instead of -EINVAL it correctly returns in such cases?  Why?

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Al Viro]

On Fri, May 10, 2024 at 01:49:06AM +0100, Al Viro wrote:
> On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote:
> > @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
> >  	struct dentry *dentry = file->f_path.dentry;
> >  	switch (whence) {
> >  		case 1:
> > -			offset += file->f_pos;
> > +			/* cannot represent offset with loff_t */
> > +			if (check_add_overflow(offset, file->f_pos, &offset))
> > +				return -EOVERFLOW;
> 
> Instead of -EINVAL it correctly returns in such cases?  Why?

We have file->f_pos in range 0..LLONG_MAX.  We are adding a value in
range LLONG_MIN..LLONG_MAX.  The sum (in $\Bbb Z$) cannot be below
LLONG_MIN or above 2 * LLONG_MAX, so if it can't be represented by loff_t,
it must have been in range LLONG_MAX + 1 .. 2 * LLONG_MAX.  Result of
wraparound would be equal to that sum - 2 * LLONG_MAX - 2, which is going
to be in no greater than -2.  We will run
			fallthrough;
		case 0:
			if (offset >= 0)
				break;
			fallthrough;
		default:
			return -EINVAL;
and fail with -EINVAL.

Could you explain why would -EOVERFLOW be preferable and why should we
engage in that bit of cargo cult?

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Justin Stitt]

Hi,

On Fri, May 10, 2024 at 02:04:51AM +0100, Al Viro wrote:
> On Fri, May 10, 2024 at 01:49:06AM +0100, Al Viro wrote:
> > On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote:
> > > @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
> > >  	struct dentry *dentry = file->f_path.dentry;
> > >  	switch (whence) {
> > >  		case 1:
> > > -			offset += file->f_pos;
> > > +			/* cannot represent offset with loff_t */
> > > +			if (check_add_overflow(offset, file->f_pos, &offset))
> > > +				return -EOVERFLOW;
> > 
> > Instead of -EINVAL it correctly returns in such cases?  Why?
> 
> We have file->f_pos in range 0..LLONG_MAX.  We are adding a value in
> range LLONG_MIN..LLONG_MAX.  The sum (in $\Bbb Z$) cannot be below
> LLONG_MIN or above 2 * LLONG_MAX, so if it can't be represented by loff_t,
> it must have been in range LLONG_MAX + 1 .. 2 * LLONG_MAX.  Result of
> wraparound would be equal to that sum - 2 * LLONG_MAX - 2, which is going
> to be in no greater than -2.  We will run
> 			fallthrough;
> 		case 0:
> 			if (offset >= 0)
> 				break;
> 			fallthrough;
> 		default:
> 			return -EINVAL;
> and fail with -EINVAL.

This feels like a case of accidental correctness. You demonstrated that
even with overflow we end up going down a control path that returns an
error code so all is good. However, I think finding the solution
shouldn't require as much mental gymnastics. We clearly don't want our
file offsets to wraparound and a plain-and-simple check for that lets
readers of the code understand this.

> 
> Could you explain why would -EOVERFLOW be preferable and why should we
> engage in that bit of cargo cult?

I noticed some patterns in fs/ regarding -EOVERFLOW and thought this was
a good application of this particular error code.

Some examples:

read_write.c::do_sendfile()
  ...
	if (unlikely(pos + count > max)) {
		retval = -EOVERFLOW;
		if (pos >= max)
			goto fput_out;
		count = max - pos;
	}

read_write.c::generic_copy_file_checks()
  ...
	/* Ensure offsets don't wrap. */
	if (pos_in + count < pos_in || pos_out + count < pos_out)
		return -EOVERFLOW;

... amongst others.

So to answer your question, I don't have strong feelings about what the
error code should be; I was just attempting to match patterns I had seen
within this section of the codebase when handling overflow/wraparound.

If -EOVERFLOW is technically incorrect or if it is just bad style, I'm
happy to send a new version swapping it over to -EINVAL

Thanks
Justin

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Al Viro]

On Fri, May 10, 2024 at 03:26:08AM +0000, Justin Stitt wrote:

> This feels like a case of accidental correctness. You demonstrated that
> even with overflow we end up going down a control path that returns an
> error code so all is good.

No.  It's about a very simple arithmetical fact: the smallest number that
wraps to 0 is 2^N, which is more than twice the maximal signed N-bit
value.  So wraparound on adding a signed N-bit to non-negative signed N-bit
will always end up with negative result.  That's *NOT* a hard math.  Really.

As for the rest... SEEK_CUR semantics is "seek to current position + offset";
just about any ->llseek() instance will have that shape - calculate the
position we want to get to, then forget about the difference between
SEEK_SET and SEEK_CUR.  So noticing that wraparound ends with negative
is enough - we reject straight SEEK_SET to negatives anyway, so no
extra logics is needed.

> However, I think finding the solution
> shouldn't require as much mental gymnastics. We clearly don't want our
> file offsets to wraparound and a plain-and-simple check for that lets
> readers of the code understand this.

No comments that would be suitable for any kind of polite company.

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Al Viro]

On Fri, May 10, 2024 at 05:48:05AM +0100, Al Viro wrote:
> On Fri, May 10, 2024 at 03:26:08AM +0000, Justin Stitt wrote:
> 
> > This feels like a case of accidental correctness. You demonstrated that
> > even with overflow we end up going down a control path that returns an
> > error code so all is good.
> 
> No.  It's about a very simple arithmetical fact: the smallest number that
> wraps to 0 is 2^N, which is more than twice the maximal signed N-bit
> value.  So wraparound on adding a signed N-bit to non-negative signed N-bit
> will always end up with negative result.  That's *NOT* a hard math.  Really.
> 
> As for the rest... SEEK_CUR semantics is "seek to current position + offset";
> just about any ->llseek() instance will have that shape - calculate the
> position we want to get to, then forget about the difference between
> SEEK_SET and SEEK_CUR.  So noticing that wraparound ends with negative
> is enough - we reject straight SEEK_SET to negatives anyway, so no
> extra logics is needed.
> 
> > However, I think finding the solution
> > shouldn't require as much mental gymnastics. We clearly don't want our
> > file offsets to wraparound and a plain-and-simple check for that lets
> > readers of the code understand this.
> 
> No comments that would be suitable for any kind of polite company.

FWIW, exchange of nasty cracks aside, I believe that this kind of
whack-a-mole in ->llseek() instances is just plain wrong.  We have
80-odd instances in the tree.

Sure, a lot of them a wrappers for standard helpers, but that's
still way too many places to spill that stuff over.  And just
about every instance that supports SEEK_CUR has exact same kind
of logics.

As the matter of fact, it would be interesting to find out
which instances, if any, do *not* have that relationship
between SEEK_CUR and SEEK_SET.  If such are rare, it might
make sense to mark them as such in file_operations and
have vfs_llseek() check that - it would've killed a whole
lot of boilerplate.  And there it a careful handling of
overflow checks (or a clear comment explaining what's
going on) would make a lot more sense.

IF we know that an instance deals with SEEK_CUR as SEEK_SET to
offset + ->f_pos, we can translate SEEK_CUR into SEEK_SET
in the caller.

Re: [PATCH] libfs: fix accidental overflow in offset calculation

[Al Viro]

On Fri, May 10, 2024 at 07:33:12AM +0100, Al Viro wrote:

> As the matter of fact, it would be interesting to find out
> which instances, if any, do *not* have that relationship
> between SEEK_CUR and SEEK_SET.  If such are rare, it might
> make sense to mark them as such in file_operations and
> have vfs_llseek() check that - it would've killed a whole
> lot of boilerplate.  And there it a careful handling of
> overflow checks (or a clear comment explaining what's
> going on) would make a lot more sense.
> 
> IF we know that an instance deals with SEEK_CUR as SEEK_SET to
> offset + ->f_pos, we can translate SEEK_CUR into SEEK_SET
> in the caller.

FWIW, weird instances do exist.

kernel/printk/printk.c:devkmsg_llseek(), for example.  Or this
gem in drivers/fsi/i2cr-scom.c:
static loff_t i2cr_scom_llseek(struct file *file, loff_t offset, int whence)
{
        switch (whence) {
        case SEEK_CUR:
                break;
        case SEEK_SET:
                file->f_pos = offset;
                break;
        default:
                return -EINVAL;
        }

        return offset;
}
SEEK_CUR handling in particular is just plain bogus: lseek(fd, -9, SEEK_CUR)
doing nothing to current position and returning EBADF.  Even if you've done
lseek(fd, 9, SEEK_SET) just before that...

I suspect that some of those might be outright bugs; /dev/kmsg one probably
isn't, but by the look of it those should be rare.

Then there's orangefs_dir_llseek(), with strange handling of SEEK_SET
(but not SEEK_CUR); might or might not be a bug...

From the quick look it does appear that it might be a project worth
attempting - exceptions are very rare.