Re: [PATCH 06/11] ksmbd: fix subauth 0 handling in sid_to_id()

[Christian Brauner <christian.brauner@ubuntu.com>]

On Tue, Aug 24, 2021 at 05:13:01PM +0900, Namjae Jeon wrote:
> 2021-08-24 0:13 GMT+09:00, Christian Brauner <brauner@kernel.org>:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > It's not obvious why subauth 0 would be excluded from translation. This
> > would lead to wrong results whenever a non-identity idmapping is used.
> >
> > Cc: Steve French <stfrench@microsoft.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Namjae Jeon <namjae.jeon@samsung.com>
> > Cc: Hyunchul Lee <hyc.lee@gmail.com>
> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Cc: linux-cifs@vger.kernel.org
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  fs/ksmbd/smbacl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> > index 3307ca776eb1..0d269b28f163 100644
> > --- a/fs/ksmbd/smbacl.c
> > +++ b/fs/ksmbd/smbacl.c
> > @@ -274,7 +274,7 @@ static int sid_to_id(struct user_namespace *user_ns,
> >  		uid_t id;
> >
> >  		id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]);
> > -		if (id > 0) {
> > +		if (id >= 0) {
> >  			uid = make_kuid(user_ns, id);
> >  			if (uid_valid(uid) && kuid_has_mapping(user_ns, uid)) {
> >  				fattr->cf_uid = uid;
> > @@ -286,9 +286,9 @@ static int sid_to_id(struct user_namespace *user_ns,
> >  		gid_t id;
> >
> >  		id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]);
> > -		if (id > 0) {
> >  			gid = make_kgid(user_ns, id);
> >  			if (gid_valid(gid) && kgid_has_mapping(user_ns, gid)) {
> > +		if (id >= 0) {
> Checkpatch.pl give warning messages like the following :
> 
> WARNING: suspect code indent for conditional statements (24, 16)
> #110: FILE: fs/ksmbd/smbacl.c:290:
>  			if (gid_valid(gid) && kgid_has_mapping(user_ns, gid)) {
> +		if (id >= 0) {
> 
> WARNING: suspect code indent for conditional statements (16, 32)
> #111: FILE: fs/ksmbd/smbacl.c:291:
> +		if (id >= 0) {
>  				fattr->cf_gid = gid;
> 
> With 7th patch, it shouldn't be a problem, but this patch itself seems
> to have a problem.
> I will directly update it like this if you don't mind :
> 
>                 id = le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]);
> -               if (id > 0) {
> +               if (id >= 0) {
>                         gid = make_kgid(user_ns, id);
>                         if (gid_valid(gid) && kgid_has_mapping(user_ns, gid)) {
>                                 fattr->cf_gid = gid;
>                                 rc = 0;
>                         }
> 

Sounds good. Thanks!

Christian