Bug 218592 - [patch] fsck_ffs(8): incorrect bounds check in preen mode when softdep is enabled
Summary: [patch] fsck_ffs(8): incorrect bounds check in preen mode when softdep is ena...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: FreeBSD bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-04-12 15:20 UTC by todd.miller
Modified: 2017-04-21 10:17 UTC (History)
3 users (show)

See Also:


Attachments
Prevent inosused from wrapping (769 bytes, patch)
2017-04-12 15:20 UTC, todd.miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description todd.miller 2017-04-12 15:20:43 UTC
Created attachment 181719 [details]
Prevent inosused from wrapping

The loop that scans the used inode map when soft updates is in use assumes that the inosused variable is signed.  However, ino_t is unsigned, so the loop invariant is incorrect and the check for inosused wrapping to < 0 can never be true.

Instead of checking for wrap after the fact it is possible to just prevent it from happening in the first place, which the attached patch does.  I've just committed a similar fix to OpenBSD.
Comment 1 Konstantin Belousov freebsd_committer 2017-04-12 21:46:52 UTC
The patch looks fine to me.
Comment 2 Kirk McKusick freebsd_committer 2017-04-12 23:18:53 UTC
The patch looks fine to me.

I'll note that inosused will always be a multiple of CHAR_BIT, so in fact the loop will always work. However, I am a strong believer in having code that will work absent these "known" invariants, so am in favor of the change.
Comment 3 Konstantin Belousov freebsd_committer 2017-04-13 12:46:34 UTC
(In reply to Kirk McKusick from comment #2)
Note that inoused is read from superblock.  If we ever get into the situation when the counter becomes negative, should we abort preen mode ?
Comment 4 commit-hook freebsd_committer 2017-04-14 15:22:23 UTC
A commit references this bug:

Author: kib
Date: Fri Apr 14 15:22:01 UTC 2017
New revision: 316852
URL: https://svnweb.freebsd.org/changeset/base/316852

Log:
  In fsck_ffs pass1, prevent the inosused variable from wrapping.

  The loop that scans the used inode map when soft updates is in use
  assumes that the inosused variable is signed.  However, ino_t is
  unsigned, so the loop invariant is incorrect and the check for
  inosused wrapping to < 0 can never be true.

  Instead of checking for wrap after the fact just prevent it from
  happening in the first place.

  PR:	218592
  Submitted by:	Todd Miller <todd.miller@courtesan.com>
  Reviewed by:	mckusick
  MFC after:	1 week

Changes:
  head/sbin/fsck_ffs/pass1.c
Comment 5 commit-hook freebsd_committer 2017-04-21 10:13:56 UTC
A commit references this bug:

Author: kib
Date: Fri Apr 21 10:13:08 UTC 2017
New revision: 317249
URL: https://svnweb.freebsd.org/changeset/base/317249

Log:
  MFC r316852:
  In fsck_ffs pass1, prevent the inosused variable from wrapping.

  PR:	218592

Changes:
_U  stable/11/
  stable/11/sbin/fsck_ffs/pass1.c
Comment 6 commit-hook freebsd_committer 2017-04-21 10:17:01 UTC
A commit references this bug:

Author: kib
Date: Fri Apr 21 10:16:35 UTC 2017
New revision: 317250
URL: https://svnweb.freebsd.org/changeset/base/317250

Log:
  MFC r316852:
  In fsck_ffs pass1, prevent the inosused variable from wrapping.

  PR:	218592

Changes:
_U  stable/10/
  stable/10/sbin/fsck_ffs/pass1.c