linux-kernel - Re: [PATCH v5 11/14] tools/nolibc: clean up mmap() support

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: <20230702192347.GJ16233@1wt.eu>
Date:   Sun, 2 Jul 2023 21:23:47 +0200
From:   Willy Tarreau <w@....eu>
To:     Zhangjin Wu <falcon@...ylab.org>
Cc:     thomas@...ch.de, arnd@...db.de, david.laight@...lab.com,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v5 11/14] tools/nolibc: clean up mmap() support

On Wed, Jun 28, 2023 at 09:41:13PM +0800, Zhangjin Wu wrote:
>  static __attribute__((unused))
>  void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
>  {
> -	void *ret = sys_mmap(addr, length, prot, flags, fd, offset);
> -
> -	if ((unsigned long)ret >= -4095UL) {
> -		SET_ERRNO(-(long)ret);
> -		ret = MAP_FAILED;
> -	}
> -	return ret;
> +	return (void *)__sysret((unsigned long)sys_mmap(addr, length, prot, flags, fd, offset));
>  }

One point regarding this one. By doing so, we're hard-coding the fact
that we consider that MAP_FAILED is always -1. I'm not necessarily
against it, but this implication can be confusing for those searching
where it's being set. I would suggest putting a comment before the
mmap() function saying:

 /* Note that on Linux MAP_FAILED is -1 so we can use the generic __sysret()
  * which returns -1 upon error and still satisfy user land that checks for
  * MAP_FAILED.
  */

Since it's an assumed choice that theoretically could affect portability,
it should be reflected in the commit message as well (and we all know it
does not have any impact).

Thanks!
Willy