linux-kernel - Re: [GIT PULL] fscrypt fix for v6.3-rc4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230320225934.GB21979@sol.localdomain>
Date:   Mon, 20 Mar 2023 15:59:34 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>
Subject: Re: [GIT PULL] fscrypt fix for v6.3-rc4

On Mon, Mar 20, 2023 at 03:16:48PM -0700, Linus Torvalds wrote:
> On Mon, Mar 20, 2023 at 1:56 PM Eric Biggers <ebiggers@...nel.org> wrote:
> >
> >       fscrypt: check for NULL keyring in fscrypt_put_master_key_activeref()
> 
> Side note: please just use WARN_ON_ONCE() for things like this, not WARN_ON.
> 
> If it's triggerable, it should be triggered only once rather than
> flood the logs and possibly cause a DoS.
> 
> And if it's not triggerable, the "ONCE" doesn't matter.
> 
> I note that fscypt in general seems to be *way* too happy with
> WARN_ON() as some kind of debugging aid.
> 
> It's not good in general (printf for debugging is wonderful, but
> shouldn't be left in the sources to rot for all eternity), but it's
> particularly not good in that form.
> 

Yes, I agree that most of the WARN_ONs should be WARN_ON_ONCEs.  I think I've
been assuming that WARN_ON is significantly more lightweight than WARN_ON_ONCE.
But that doesn't seem to be the case, especially since commit 19d436268dde
("debug: Add _ONCE() logic to report_bug()").

But besides that, I believe WARN* is generally being used appropriately in
fs/crypto/.  It's used when assumptions made by the code are violated, but where
the hard crash of a BUG() is not necessary.  I think this is a good thing to
have, versus the alternative of doing nothing and making it much harder to track
down bugs...  Some particularly bad crypto bugs that we can easily WARN about,
such as IVs being truncated, may not even be detectable by users otherwise.

There are probably a few that should be removed, though.  I'm also considering
whether the refcounting-related ones should use CHECK_DATA_CORRUPTION, though
that may run afoul of the "don't use BUG() unless absolutely needed" rule...

- Eric