linux-kernel - Re: [PATCH] Check for Null return of function of affs_bread in function affs

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: <20140622191248.GU18016@ZenIV.linux.org.uk>
Date:	Sun, 22 Jun 2014 20:12:48 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Nick Krause <xerofoify@...il.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	akpm@...ux-foundation.org, fabf@...net.be,
	kirill.shutemov@...ux.intel.com, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Check for Null return of function of affs_bread in
 function affs_truncate

On Sat, Jun 21, 2014 at 01:59:15AM +0200, Thomas Gleixner wrote:
> > > The problem is that we don't know if we should return here or break
> > > here.  If you don't understand the code, then it's best to just leave it
> > > alone.
> 
> Dan, what kind of attitude is that?
>
> Nick certainly found an issue where a possible NULL return from
> affs_bread() can cause havoc.

Yes.  No arguments here.

> Do YOU understand that code?
> 
> If yes, you better explain, WHY Nicks finding is a false positive
> instead of just telling him off in a very inpolite way.

It's not a false positive at all.

> If not, you better refrain from telling a reporter that he does not
> understand the code and should stay away.

Tone aside, he does have a point - namely, that patch in question doesn't
contain any analysis of the bug and recovery strategy, turning a bug into
something that is much harder to spot on inspection.

If nothing else, such patches should contain a loud printk added on
the b0rken codepath, along with a big fat warning in the source.

I'm not saying that "fuck off unless you understand the code" is a sane
policy - it's not.  But "anything's better than an oops" is also wrong.

> > > The problem is that we don't know if we should return here or break
> > > here.
> 
> The problem here is that proceeding with a known NULL pointer is wrong
> to begin with. It does not matter at all whether break or return is
> the proper thing to do. What matters is that proceeding with a NULL
> pointer is wrong to begin with, no matter what.
> 
> So either explain why this is a non issue and the NULL pointer return
> cannot happen or shut up and try to find a proper solution for that
> "return" vs. "break" issue.

return vs. break here is the difference between discarding preallocated blocks
and leaking them as well.  The thing is, it's either severe OOM or a corrupted
image.  I'd *probably* go for "affs_error() and return" here, but that's
not the only question - we probably ought to make it return an error, instead
of having it void.  And callers tend to do that affs_free_prealloc(), so
much that I'm not sure if we actually want to keep it in affs_truncate() at
all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/