linux-kernel - Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

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: <d91debb6-2075-4e1a-af0c-2f3a658cabb3@oracle.com>
Date: Fri, 29 Nov 2024 22:42:47 +0100
From: Vegard Nossum <vegard.nossum@...cle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>
Cc: Kees Cook <kees@...nel.org>, linux-kernel@...r.kernel.org,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Nir Lichtman
 <nir@...htman.org>,
        Tycho Andersen <tandersen@...flix.com>
Subject: Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)


On 29/11/2024 05:44, Linus Torvalds wrote:
> On Thu, 28 Nov 2024 at 19:34, Al Viro <viro@...iv.linux.org.uk> wrote:
>>
>> Just one thing - IMO we want to use the relative pathname when it's
>> not empty.  Even in execveat()
> 
> Oh, absolutely agreed.
> 
> Good catch, because yes, I messed that part up in my suggested patch at
> 
>     https://lore.kernel.org/all/CAHk-=wjF_09Z6vu7f8UAbQVDDoHnd-j391YpUxmBPLD=SKbKtQ@mail.gmail.com/
> 
> which did this dentry name thing for anything that used a base fd, but
> yes, as you say, it should only do it when there is no name at all.
> 
> So instead of basing it (incorrectly) on that existing
> 
>          if (fd == AT_FDCWD || filename->name[0] == '/') {
> 
> test, the logic should probably look something like
> 
>          if (!filename->name[0]) {
>                  rcu_read_lock();
>                  strscpy(bprm->comm,
> smp_load_acquire(&file->f_path.dentry->d_name.name));
>                  rcu_read_unlock();
>          } else
>                  strscpy(bprm->comm, kbasename(filename->name));
> 
> and it probably wouldn't be a bad idea to separate this out to be a
> helper function that just does this one thing.

Probably a silly question, but why not do the same thing in all cases?
file->f_path.dentry->d_name.name _is_ the basename of whatever the
normal path resolution machinery ended up with; why not use just it
unconditionally? It handled empty paths already. This also seems to
match what you wrote in 
https://lore.kernel.org/all/CAHk-=wgKgi5eqo6oW0bBS2-Cr+d4jraoKfVq6wbmdiWWsZbMLw@mail.gmail.com/ 
(no argv[0], no magic strings, no symlinks):

> dentry->d_name really *IS* the name of the file that is associated 
> with the 'fd'.

Vegard