linux-kernel - Re: [PATCH v4] kernel: make groups_sort calling a responsibility group

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: <20171211142708.GA23284@bombadil.infradead.org>
Date:   Mon, 11 Dec 2017 06:27:08 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Thiago Rafael Becker <thiago.becker@...il.com>
Cc:     viro@...iv.linux.org.uk, schwidefsky@...ibm.com,
        bfields@...ldses.org, neilb@...e.com, linux-nfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] kernel: make groups_sort calling a responsibility
 group_info allocators

On Mon, Dec 11, 2017 at 11:28:06AM -0200, Thiago Rafael Becker wrote:
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
>  				gi->gid[i] = exp->ex_anon_gid;
>  			else
>  				gi->gid[i] = rqgi->gid[i];
> +
> +			/* Should be race free as long as each thread allocates a new gi */
> +			groups_sort(gi);
>  		}

Overlong line.  Would recommend:

			/* Each thread has its own gi, so no race */

> +++ b/kernel/groups.c
> @@ -86,11 +86,13 @@ static int gid_cmp(const void *_a, const void *_b)
>  	return gid_gt(a, b) - gid_lt(a, b);
>  }
>  
> -static void groups_sort(struct group_info *group_info)
> +void groups_sort(struct group_info *group_info)
>  {
>  	sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
>  	     gid_cmp, NULL);
>  }
> +EXPORT_SYMBOL(groups_sort);
> +
>  

Spurious extra line

> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..7154dab 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -520,6 +520,12 @@ static int unix_gid_parse(struct cache_detail *cd,
>  		ug.gi->gid[i] = kgid;
>  	}
>  
> +	/* Sort the groups before inserting this entry
> +	 * into the cache to avoid future corrutpions
> +	 * by multiple simultaneous attempts to sort this
> +	 * entry.
> +	 */
> +	groups_sort(ug.gi);
>  	ugp = unix_gid_lookup(cd, uid);
>  	if (ugp) {
>  		struct cache_head *ch;

Why comment this call and not the other ones?  I appreciate this is the
call-site where you discovered the bug, but that's not going to make it
special to someone who's reading this code in ten years time.  I would
leave this comment out entirely; it's just the new way we do things.

I can't find anything else to critique; nice job.