linux-kernel - Re: [PATCH v2] erofs: fix incorrect symlink detection in fast symlink

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: <ba83ef6e-d4cc-4ade-9dd0-e3fdfa8fde70@app.fastmail.com>
Date: Tue, 10 Sep 2024 16:51:41 -0400
From: "Colin Walters" <walters@...bum.org>
To: "Gao Xiang" <hsiangkao@...ux.alibaba.com>, linux-erofs@...ts.ozlabs.org
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] erofs: fix incorrect symlink detection in fast symlink



On Mon, Sep 9, 2024, at 10:18 PM, Gao Xiang wrote:

> I know you ask for an explicit check on symlink i_size, but
> I've explained the current kernel behavior:
>    - For symlink i_size < PAGE_SIZE (always >= 4096 on Linux),
>      it behaves normally for EROFS Linux implementation;
>
>    - For symlink i_size >= PAGE_SIZE, EROFS Linux
>      implementation will mark '\0' at PAGE_SIZE - 1 in
>      page_get_link() -> nd_terminate_link() so the behavior is also
>      deterministic and not harmful to the system stability and security;

Got it, OK.

> In other words, currently i_size >= PAGE_SIZE is an undefined behavior
> but Linux just truncates the link path.

I think where we had a miscommunication is that when I see "undefined behavior" I thought you were using the formal term: https://en.wikipedia.org/wiki/Undefined_behavior

The term for what you're talking about in my experience is usually "unspecified behavior" or "implementation defined behavior" which (assuming a reasonable implementor) would include silent truncation or an explicit error, but *not* walking off the end of a buffer and writing to arbitrary other kernel memory etc.

(Hmm really given the widespread use of nd_terminate_link I guess this is kind of more of a "Linux convention" than just an EROFS one, with XFS as a notable exception?)

> For this case, to be clear I'm totally fine with the limitation,
> but I need to decide whether I should make "EROFS_SYMLINK_MAXLEN"
> as 4095 or "EROFS_SYMLINK_MAXLEN" as 4096 but also accepts
> `link[4095] == '\0'`.

Mmmm...I think PATH_MAX is conventionally taken to include the NUL; yeah see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/namei.c?id=b40c8e7a033ff2cafd33adbe50e2a516f88fa223#n123