linux-kernel - Re: [PATCH v2 2/2] mm: fix boundary judgment issues in kernel/resource.c

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: <87k0mph54i.ffs@nanos.tec.linutronix.de>
Date:   Sun, 20 Jun 2021 00:16:45 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Yaohui Wang <yaohuiwang@...ux.alibaba.com>,
        dave.hansen@...ux.intel.com
Cc:     luto@...nel.org, peterz@...radead.org, mingo@...hat.com,
        bp@...en8.de, x86@...nel.org, linux-kernel@...r.kernel.org,
        Ben Luo <luoben@...ux.alibaba.com>,
        Yahui Wang <yaohuiwang@...ux.alibaba.com>
Subject: Re: [PATCH v2 2/2] mm: fix boundary judgment issues in kernel/resource.c

Yaohui!

On Fri, Jun 11 2021 at 12:21, Yaohui Wang wrote:

The same formal issues as with patch #1

> The original boundary judgment may ignore @end if @end equals @start. For

May means it can but it must not. But this is not the case here. end
equals start is always ignored.

Also 'original' is meaningless here. Before the patch is applied the
code is that way.

 find_next_iomem_res() and __walk_iomem_res_desc() require that the
 provided end address is larger than the start address, which ...

> example, if we call ioremap(phys, 1), then @end == @start, and the memory
> check will not be applied on the page where @end lives, which is
> unexpected.

Please avoid 'we' and 'I':

 is incorrect when ioremap() is invoked with length=1.

> In kernel/resource.c:find_next_iomem_res, the mem region is a closed

See the reply to #1 vs. function names. Also please write out 'memory',
there is no shortage of space in change logs.

> interval (i.e. [@start..@end]). So @start == @end should be allowed.

closed interval reads strange. The usual terminology is: The end address
is inclusive.

  Resources are described with the start address and the inclusive end
  address, which means for a resource with 1 byte length the start
  address is the same as the end address.

  find_next_iomem_res() and __walk_iomem_res_desc() ignore resources
  with 1 byte length, which prevents that ioremap(phys, 1) is checked
  whether it touches non ioremappable resources.

  ...

Thanks,

        tglx