| Message ID | YO8Rw23KxCDjzKeA@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] configfs fix for Linux 5.14 | expand |
On Wed, Jul 14, 2021 at 9:33 AM Christoph Hellwig <hch@infradead.org> wrote: > > configfs fix for Linux 5.14 > > - fix the read and write iterators (Bart Van Assche) I've pulled this, but I'm somewhat disgusted by it. The overflow "protection" is just wrong: + to_copy = SIMPLE_ATTR_SIZE - 1 - pos; + if (to_copy <= 0) + return 0; because if users control "pos", then that "to_copy" could be a huge positive value even after overflow protection. I hope/think that we always end up checking 'pos' in the VFS layer so that this isn't a bug in practice, but people - the above is just fundamentally bad code. It's simply not the correct way to check limits. It does it badly, and it's hard to read (*). If you want to check limits, then do it (a) the obvious way and (b) right. Something like if (pos < 0 || pos >= SIMPLE_ATTR_SIZE - 1) return 0; to_copy = SIMPLE_ATTR_SIZE - 1 - pos; would have been a hell of a lot more obvious, would have been CORRECT, and a compiler would likely be able to equally good code for it. Doing a "x <0 || x > C" test is actually nice and cheap, and compilers should all be smart enough to turn it into a single (unsigned) comparison. Possibly it even generates better code, since "to_copy" could then - and should - no longer be a 64-bit loff_t, since it's pointless. We've just checked the range of the values, so it can be the natural size for the machine. Although from a small test, gcc does seem to be too simple to take advantage of that, and on 32-bit x86 it does the range check using 64-bit arithmetic even when unnecessary (it should just check "are the upper 32 bits zero" rather than play around with doing a 64-bit sub/sbb - I'm surprised, because I thought gcc already knew about this, but maybe compiler people are starting to forget about 32-bit stuff too). But even if the compiler doesn't figure it out, the simple "just check the limits" is a lot more readable for humans, and avoids the whole overflow issue. And maybe some compilers will do better at it. Linus (*) Ok, it's easy to read if you ignore the overflow possibility. IOW, it's easy to read WRONG.
On Wed, Jul 14, 2021 at 1:05 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I hope/think that we always end up checking 'pos' in the VFS layer so > that this isn't a bug in practice Yeah, we seem to make sure everything is fine in rw_verify_area(). We do allow negative 'pos' things, but only for files marked with FMODE_UNSIGNED_OFFSET, which is basically just for variations of /dev/mem and /proc/<pid>/mem that need the whole 64-bit range. So it _shouldn't_ be an issue here, but the points about just doing the legible and safe version stands. Linus
The pull request you sent on Wed, 14 Jul 2021 18:33:07 +0200:
> git://git.infradead.org/users/hch/configfs.git tags/configfs-5.13-1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/1013d4add290c460b816fc4b1db5174f88b71760
Thank you!
The following changes since commit 7fef2edf7cc753b51f7ccc74993971b0a9c81eca: sd: don't mess with SD_MINORS for CONFIG_DEBUG_BLOCK_EXT_DEVT (2021-07-12 12:25:37 -0700) are available in the Git repository at: git://git.infradead.org/users/hch/configfs.git tags/configfs-5.13-1 for you to fetch changes up to 420405ecde061fde76d67bd3a67577a563ea758e: configfs: fix the read and write iterators (2021-07-13 20:56:24 +0200) ---------------------------------------------------------------- configfs fix for Linux 5.14 - fix the read and write iterators (Bart Van Assche) ---------------------------------------------------------------- Bart Van Assche (1): configfs: fix the read and write iterators fs/configfs/file.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)