linux-kernel - Re: [PATCH 2/2] coredump: fix PIDFD_INFO

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250704-badeanstalt-eurem-b944cdc46c4c@brauner>
Date: Fri, 4 Jul 2025 10:57:27 +0200
From: Christian Brauner <brauner@...nel.org>
To: Laura Brehm <laurajfbrehm@...il.com>
Cc: linux-kernel@...r.kernel.org, Laura Brehm <laurabrehm@....com>, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 2/2] coredump: fix PIDFD_INFO_COREDUMP ioctl check

On Thu, Jul 03, 2025 at 02:02:44PM +0200, Laura Brehm wrote:
> In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs,
> coredump: add PIDFD_INFO_COREDUMP"), the following code was added:
> 
>     if (mask & PIDFD_INFO_COREDUMP) {
>         kinfo.mask |= PIDFD_INFO_COREDUMP;
>         kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
>     }
>     [...]
>     if (!(kinfo.mask & PIDFD_INFO_COREDUMP)) {
>         task_lock(task);
>         if (task->mm)
>             kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
>         task_unlock(task);
>     }
> 
> The second bit in particular looks off to me - the condition in essence
> checks whether PIDFD_INFO_COREDUMP was **not** requested, and if so
> fetches the coredump_mask in kinfo, since it's checking !(kinfo.mask &
> PIDFD_INFO_COREDUMP), which is unconditionally set in the earlier hunk.
> 
> I'm tempted to assume the idea in the second hunk was to calculate the
> coredump mask if one was requested but fetched in the first hunk, in
> which case the check should be
>     if ((kinfo.mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))
> which might be more legibly written as
>     if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))
> 
> This could also instead be achieved by changing the first hunk to be:
> 
>     if (mask & PIDFD_INFO_COREDUMP) {
> 	kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
> 	if (kinfo.coredump_mask)
> 	    kinfo.mask |= PIDFD_INFO_COREDUMP;
>     }
> 
> and the second hunk to:
> 
>     if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.mask & PIDFD_INFO_COREDUMP)) {
> 	task_lock(task);
>         if (task->mm) {
> 	    kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
>             kinfo.mask |= PIDFD_INFO_COREDUMP;
>         }
>         task_unlock(task);
>     }
> 
> However, when looking at this, the supposition that the second hunk
> means to cover cases where the coredump info was requested but the first
> hunk failed to get it starts getting doubtful, so apologies if I'm
> completely off-base.
> 
> This patch addresses the issue by fixing the check in the second hunk.
> 
> Signed-off-by: Laura Brehm <laurabrehm@....com>
> Cc: brauner@...nel.org
> Cc: linux-fsdevel@...r.kernel.org
> ---

Yes, that looks correct to me. Thanks for the fix!