linux-kernel - Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping page unaligned ranges

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] [day] [month] [year] [list]

Message-ID: <20160923162307.GB31387@htj.duckdns.org>
Date:   Fri, 23 Sep 2016 12:23:07 -0400
From:   Tejun Heo <tj@...nel.org>
To:     zijun_hu <zijun_hu@...o.com>
Cc:     zijun_hu@....com, Andrew Morton <akpm@...ux-foundation.org>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org, mingo@...nel.org,
        rientjes@...gle.com, iamjoonsoo.kim@....com,
        mgorman@...hsingularity.net
Subject: Re: [PATCH 1/1] lib/ioremap.c: avoid endless loop under ioremapping
 page unaligned ranges

Hello,

On Fri, Sep 23, 2016 at 11:41:33PM +0800, zijun_hu wrote:
> 1. ioremap_page_range() is not a kernel internal function

What matters is whether the input is from userland or easy to get
wrong (IOW the checks serve practical purposes).  Whether a function
is exported via EXPORT_SYMBOL doesn't matter that much in this regard.
EXPORT_SYMBOL doesn't really demark an API layer (we don't put any
effort into keeping it stable or versioned).  Modules are loaded
separately but still part of the same program.

> 2. sanity check "BUG_ON(addr >= end)" have existed already, but don't check enough

That particular check being there doesn't imply that it needs to be
expanded.  If you actually have cases where this mattered and extra
checks would have substantially helped debugging, sure, but that's not
the case here.

> 3. are there any obvious hint for rang parameter requirements but BUG_ON(addr >= end)

You're welcome to add documentation.

> 4. if range which seems right but wrong really is used such as mapping 
>    virtual range [0x80000800, 0x80007800) to physical area[0x20000800, 0x20007800)
>    what actions should we take? warning message and trying to finish user request
>    or panic kernel or hang system in endless loop or return -EINVAL？
>    how to help user find their problem?
> 5. if both boundary of the range are aligned to page, ioremap_page_range() works well
>    otherwise endless loop maybe happens

I don't think it's helpful to imgaine pathological conditions and try
to address all of them.  There's no evidence, not even a tenuous one,
that anyone is suffering from this.  Sometimes it is useful to
implement precautions preemptively but in those cases the rationles
should be along the line of "it's easy to get the inputs wrong for
this function because ABC and those cases are difficult to debug due
to XYZ", not "let's harden all the functions against all input
combinations regardless of likelihood or usefulness".

The thing is that the latter approach not only wastes time of everyone
involved without any real gain but also actually deteriorates the code
base by adding unnecessary complications and introduces bugs through
gratuitous changes.  Please note that I'm not trying to say
re-factoring or cleanups are to be avoided.  We need them for long
term maintainability, even at the cost of introducing some bugs in the
process, but the patches you're submitting are quite off the mark.

Thanks.

-- 
tejun