All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Justin Stitt <justinstitt@google.com>
Cc: Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
	Nathan Chancellor <nathan@kernel.org>,
	Bill Wendling <morbo@google.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] libfs: fix accidental overflow in offset calculation
Date: Fri, 10 May 2024 05:48:05 +0100	[thread overview]
Message-ID: <20240510044805.GW2118490@ZenIV> (raw)
In-Reply-To: <6oq7du4gkj3mvgzgnmqn7x44ccd3go2d22agay36chzvuv3zyt@4fktkazj4cvw>

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.

  reply	other threads:[~2024-05-10  4:48 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-10  0:35 [PATCH] libfs: fix accidental overflow in offset calculation Justin Stitt
2024-05-10  0:49 ` Al Viro
2024-05-10  1:04   ` Al Viro
2024-05-10  3:26     ` Justin Stitt
2024-05-10  4:48       ` Al Viro [this message]
2024-05-10  6:33         ` Al Viro
2024-05-10  7:02           ` Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240510044805.GW2118490@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=brauner@kernel.org \
    --cc=jack@suse.cz \
    --cc=justinstitt@google.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=morbo@google.com \
    --cc=nathan@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.