linux-kernel - Re: [PATCH v3] mm/damon/dbgfs: avoid duplicate context directory creation

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]

Date:   Mon, 22 Aug 2022 08:24:44 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     SeongJae Park <sj@...nel.org>
Cc:     akpm@...ux-foundation.org, badari.pulavarty@...el.com,
        damon@...ts.linux.dev, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v3] mm/damon/dbgfs: avoid duplicate context directory
 creation

On Sun, Aug 21, 2022 at 06:08:53PM +0000, SeongJae Park wrote:
> From: Badari Pulavarty <badari.pulavarty@...el.com>
> 
> From: Badari Pulavarty <badari.pulavarty@...el.com>

Twice?

> 
> When user tries to create a DAMON context via the DAMON debugfs
> interface with a name of an already existing context, the context
> directory creation fails but a new context is created and added in the
> internal data structure, due to absence of the directory creation
> success check.  As a result, memory could leak and DAMON cannot be
> turned on.  An example test case is as below:
> 
>     # cd /sys/kernel/debug/damon/
>     # echo "off" >  monitor_on
>     # echo paddr > target_ids
>     # echo "abc" > mk_context
>     # echo "abc" > mk_context
>     # echo $$ > abc/target_ids
>     # echo "on" > monitor_on  <<< fails
> 
> Return value of 'debugfs_create_dir()' is expected to be ignored in
> general, but this is an exceptional case as DAMON feature is depending
> on the debugfs functionality and it has the potential duplicate name
> issue.  This commit therefore fixes the issue by checking the directory
> creation failure and immediately return the error in the case.
> 
> Fixes: 75c1c2b53c78 ("mm/damon/dbgfs: support multiple contexts")
> Cc: <stable@...r.kernel.org> # 5.15.x
> Signed-off-by: Badari Pulavarty <badari.pulavarty@...el.com>
> Signed-off-by: SeongJae Park <sj@...nel.org>
> ---
> Changes from v2
> (https://lore.kernel.org/damon/20220819171930.16166-1-sj@kernel.org/)
> - Simply check the debugfs_create_dir() return value (Andrew Morton)
> - Put a comment for justifying check of the return value (Greg KH)
> 
> Changes from v1
> (https://lore.kernel.org/damon/DM6PR11MB3978994F75A4104D714437379C679@DM6PR11MB3978.namprd11.prod.outlook.com/)
> - Manually check duplicate entry instead of checking
>   'debugfs_create_dir()' return value
> - Reword commit message and the test case
> 
> Seems Badari have some email client issue, so I (SJ) am making this
> third version of the patch based on Badari's final proposal and
> reposting on behalf of Badari.
> 
>  mm/damon/dbgfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 51d67c8050dd..3b55a1b219b5 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -818,6 +818,9 @@ static int dbgfs_mk_context(char *name)
>  		return -ENOENT;
>  
>  	new_dir = debugfs_create_dir(name, root);
> +	/* Below check is required for a potential duplicated name case */
> +	if (IS_ERR(new_dir))
> +		return PTR_ERR(new_dir);

Did you just leak the memory allocated above this check?  It's hard to
determine as you are setting global variables.

thanks,

greg k-h