Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
"J. Bruce Fields" writes:
> On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" writes:
>>
>> Not strongly. As long as we have the uid ang gid valid checks in
>> there somewhere before we start using these uids and gids much.
>> This does seems to be the case of using out of range uids and gids
>> to mean out of range uids and gids.
>>
>> In the description of my original patch before I split it up, I
>> noted that one of the small dangers might be that I had added
>> some possibly unneceessary uid and gid valid checks. So it seems I had
>> even considered this slight possibility once and noted that there was a
>> slight chance we might have to remove a few of these.
>
> Ah, I should have seen that and checked that, my bad.
I think that text was lost when I split my nfs patch into all of those
patches so I don't expect you ever saw that text.
>> It would be nice to have a comment to say that we expect this to happen
>> so people don't start wondering why there are missing checks on this
>> path.
>
> OK--something like the appended?
It looks good to me.
>> There is also a gid_valid check in the secondary uids. It looks like we
>> should keep that as we don't have any checks for invalid gids in
>> nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
>
> Honestly I don't think anyone's thought that through
I try and take these moments of that's weird to spark the thinking of
things through. Just in case there is something important was
overlooked.
> But it would seem odd to pass down an "anonymous" supplementary gid--why
> not just leave it off the list instead?
True.
> (BTW, dumb question: is it legal for userspace to use a uid -1? And was
> it illegal before the user namespace patches?)
It is now impossible and thus illegal for userspace to use a uid value
of -1. Previously to use a uid of -1 you had to jump through hoops as
some system calls would allow it (setuid) and some system calls special
cased the value of -1 (setresuid). Which mean using a uid value of -1
was while possible but a bad idea because various things would break in
strange ways.
>> Also looking towards a future in which all of these things don't reside
>> in the initial user namespace. Is mapping any out of range uid to
>> INVALID_UID and then into ex_anon_uid the always the right thing to do?
>>
>> Or is -1 a very special case in this instance. In which case we
>> probably want checks that look like:
>
> svcgssd should pass down either -1 or a valid uid, so I don't think
> we're committed to any particular handling of invalid id's other than
> -1--whatever's easiest.
Sounds good. I won't worry about them in future development then.
> commit d78f5cd062edb400190521e9540b042b4805875b
> Author: J. Bruce Fields
> Date: Mon Mar 4 08:44:01 2013 -0500
>
> nfsd: fix krb5 handling of anonymous principals
>
> krb5 mounts started failing as of
> 683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 "sunrpc: Update svcgss xdr
> handle to rpsec_contect cache".
>
> The problem is that mounts are usually done with some host principal
> which isn't normally mapped to any user, in which case svcgssd passes
> down uid -1, which the kernel is then expected to map to the
> export-specific anonymous uid or gid.
>
> The new uid_valid/gid_valid checks were therefore causing that downcall
> to fail.
>
> (Note the regression may not have been seen with older userspace that
> tended to map unknown principals to an anonymous id on their own rather
> than leaving it to the kernel.)
>
> Signed-off-by: J. Bruce Fields
Reviewed-by: "Eric W. Biederman"
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
> b/net/sunrpc/auth_gss/svcauth_gss.c
> index f7d34e7..c2ab26f 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
> else {
> int N, i;
>
> + /*
> + * NOTE: we don't check uid_valid() or gid_valid(): instead,
> + * -1 id's are later mapped to the (export-specific)
> + * anonymous id by nfsd_setuser.
> + *
> + * (But supplementary gid's get no such special
> + * treatment so are checked for validity here.)
> + */
> /* uid */
> rsci.cred.cr_uid = make_kuid(_user_ns, id);
> - if (!uid_valid(rsci.cred.cr_uid))
> - goto out;
>
> /* gid */
> if (get_int(, ))
> goto out;
> rsci.cred.cr_gid = make_kgid(_user_ns, id);
> - if (!gid_valid(rsci.cred.cr_gid))
> - goto out;
>
> /* number of additional gid's */
> if (get_int(, ))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" writes:
>
>
> > krb5 mounts started failing for me as of this patch (upstream as
> > 683428fae8c73d7d7da0fa2e0b6beb4d8df4e808),
>
> Ouch!
>
> > and I believe the problem is
> > these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
> > values to indicate "authentication succeeded, but treat this user as
> > anonymous". We delay mapping -1 id's to the (configurable-per-export)
> > anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
> >
> > if (uid_eq(new->fsuid, INVALID_UID))
> > new->fsuid = exp->ex_anon_uid;
> > if (gid_eq(new->fsgid, INVALID_GID))
> > new->fsgid = exp->ex_anon_gid;
> >
> > (We wouldn't be able to do that earlier because we don't know the export
> > yet.)
> >
> > Confirmed that the following fixes the regression.
> >
> > Eric, any objection to removing those checks?
>
> Not strongly. As long as we have the uid ang gid valid checks in
> there somewhere before we start using these uids and gids much.
> This does seems to be the case of using out of range uids and gids
> to mean out of range uids and gids.
>
> In the description of my original patch before I split it up, I
> noted that one of the small dangers might be that I had added
> some possibly unneceessary uid and gid valid checks. So it seems I had
> even considered this slight possibility once and noted that there was a
> slight chance we might have to remove a few of these.
Ah, I should have seen that and checked that, my bad.
> It would be nice to have a comment to say that we expect this to happen
> so people don't start wondering why there are missing checks on this
> path.
OK--something like the appended?
> There is also a gid_valid check in the secondary uids. It looks like we
> should keep that as we don't have any checks for invalid gids in
> nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
Honestly I don't think anyone's thought that through
But it would seem odd to pass down an "anonymous" supplementary gid--why
not just leave it off the list instead?
(BTW, dumb question: is it legal for userspace to use a uid -1? And was
it illegal before the user namespace patches?)
> Also looking towards a future in which all of these things don't reside
> in the initial user namespace. Is mapping any out of range uid to
> INVALID_UID and then into ex_anon_uid the always the right thing to do?
>
> Or is -1 a very special case in this instance. In which case we
> probably want checks that look like:
svcgssd should pass down either -1 or a valid uid, so I don't think
we're committed to any particular handling of invalid id's other than
-1--whatever's easiest.
--b.
commit d78f5cd062edb400190521e9540b042b4805875b
Author: J. Bruce Fields
Date: Mon Mar 4 08:44:01 2013 -0500
nfsd: fix krb5 handling of anonymous principals
krb5 mounts started failing as of
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 "sunrpc: Update svcgss xdr
handle to rpsec_contect cache".
The problem is that mounts are usually done with some host principal
which isn't normally mapped to any user, in which case svcgssd passes
down uid -1, which the kernel is then expected to map to the
export-specific anonymous uid or gid.
The new uid_valid/gid_valid checks were therefore causing that downcall
to fail.
(Note the regression may not have been seen with older userspace that
tended to map unknown principals to an anonymous id on their own rather
than leaving it to the kernel.)
Signed-off-by: J. Bruce Fields
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..c2ab26f 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /*
+* NOTE: we don't check uid_valid() or gid_valid(): instead,
+* -1 id's are later mapped to the (export-specific)
+* anonymous id by nfsd_setuser.
+*
+* (But supplementary gid's get no such special
+* treatment so are checked for validity here.)
+*/
/* uid */
rsci.cred.cr_uid = make_kuid(_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(, ))
goto out;
rsci.cred.cr_gid = make_kgid(_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(, ))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
J. Bruce Fields bfie...@fieldses.org writes:
krb5 mounts started failing for me as of this patch (upstream as
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808),
Ouch!
and I believe the problem is
these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
values to indicate authentication succeeded, but treat this user as
anonymous. We delay mapping -1 id's to the (configurable-per-export)
anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
if (uid_eq(new-fsuid, INVALID_UID))
new-fsuid = exp-ex_anon_uid;
if (gid_eq(new-fsgid, INVALID_GID))
new-fsgid = exp-ex_anon_gid;
(We wouldn't be able to do that earlier because we don't know the export
yet.)
Confirmed that the following fixes the regression.
Eric, any objection to removing those checks?
Not strongly. As long as we have the uid ang gid valid checks in
there somewhere before we start using these uids and gids much.
This does seems to be the case of using out of range uids and gids
to mean out of range uids and gids.
In the description of my original patch before I split it up, I
noted that one of the small dangers might be that I had added
some possibly unneceessary uid and gid valid checks. So it seems I had
even considered this slight possibility once and noted that there was a
slight chance we might have to remove a few of these.
Ah, I should have seen that and checked that, my bad.
It would be nice to have a comment to say that we expect this to happen
so people don't start wondering why there are missing checks on this
path.
OK--something like the appended?
There is also a gid_valid check in the secondary uids. It looks like we
should keep that as we don't have any checks for invalid gids in
nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
Honestly I don't think anyone's thought that through
But it would seem odd to pass down an anonymous supplementary gid--why
not just leave it off the list instead?
(BTW, dumb question: is it legal for userspace to use a uid -1? And was
it illegal before the user namespace patches?)
Also looking towards a future in which all of these things don't reside
in the initial user namespace. Is mapping any out of range uid to
INVALID_UID and then into ex_anon_uid the always the right thing to do?
Or is -1 a very special case in this instance. In which case we
probably want checks that look like:
svcgssd should pass down either -1 or a valid uid, so I don't think
we're committed to any particular handling of invalid id's other than
-1--whatever's easiest.
--b.
commit d78f5cd062edb400190521e9540b042b4805875b
Author: J. Bruce Fields bfie...@redhat.com
Date: Mon Mar 4 08:44:01 2013 -0500
nfsd: fix krb5 handling of anonymous principals
krb5 mounts started failing as of
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 sunrpc: Update svcgss xdr
handle to rpsec_contect cache.
The problem is that mounts are usually done with some host principal
which isn't normally mapped to any user, in which case svcgssd passes
down uid -1, which the kernel is then expected to map to the
export-specific anonymous uid or gid.
The new uid_valid/gid_valid checks were therefore causing that downcall
to fail.
(Note the regression may not have been seen with older userspace that
tended to map unknown principals to an anonymous id on their own rather
than leaving it to the kernel.)
Signed-off-by: J. Bruce Fields bfie...@redhat.com
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..c2ab26f 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /*
+* NOTE: we don't check uid_valid() or gid_valid(): instead,
+* -1 id's are later mapped to the (export-specific)
+* anonymous id by nfsd_setuser.
+*
+* (But supplementary gid's get no such special
+* treatment so are checked for validity here.)
+*/
/* uid */
rsci.cred.cr_uid = make_kuid(init_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(mesg, id))
goto out;
rsci.cred.cr_gid = make_kgid(init_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(mesg, N))
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
J. Bruce Fields bfie...@fieldses.org writes:
On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
J. Bruce Fields bfie...@fieldses.org writes:
Not strongly. As long as we have the uid ang gid valid checks in
there somewhere before we start using these uids and gids much.
This does seems to be the case of using out of range uids and gids
to mean out of range uids and gids.
In the description of my original patch before I split it up, I
noted that one of the small dangers might be that I had added
some possibly unneceessary uid and gid valid checks. So it seems I had
even considered this slight possibility once and noted that there was a
slight chance we might have to remove a few of these.
Ah, I should have seen that and checked that, my bad.
I think that text was lost when I split my nfs patch into all of those
patches so I don't expect you ever saw that text.
It would be nice to have a comment to say that we expect this to happen
so people don't start wondering why there are missing checks on this
path.
OK--something like the appended?
It looks good to me.
There is also a gid_valid check in the secondary uids. It looks like we
should keep that as we don't have any checks for invalid gids in
nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
Honestly I don't think anyone's thought that through
I try and take these moments of that's weird to spark the thinking of
things through. Just in case there is something important was
overlooked.
But it would seem odd to pass down an anonymous supplementary gid--why
not just leave it off the list instead?
True.
(BTW, dumb question: is it legal for userspace to use a uid -1? And was
it illegal before the user namespace patches?)
It is now impossible and thus illegal for userspace to use a uid value
of -1. Previously to use a uid of -1 you had to jump through hoops as
some system calls would allow it (setuid) and some system calls special
cased the value of -1 (setresuid). Which mean using a uid value of -1
was while possible but a bad idea because various things would break in
strange ways.
Also looking towards a future in which all of these things don't reside
in the initial user namespace. Is mapping any out of range uid to
INVALID_UID and then into ex_anon_uid the always the right thing to do?
Or is -1 a very special case in this instance. In which case we
probably want checks that look like:
svcgssd should pass down either -1 or a valid uid, so I don't think
we're committed to any particular handling of invalid id's other than
-1--whatever's easiest.
Sounds good. I won't worry about them in future development then.
commit d78f5cd062edb400190521e9540b042b4805875b
Author: J. Bruce Fields bfie...@redhat.com
Date: Mon Mar 4 08:44:01 2013 -0500
nfsd: fix krb5 handling of anonymous principals
krb5 mounts started failing as of
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 sunrpc: Update svcgss xdr
handle to rpsec_contect cache.
The problem is that mounts are usually done with some host principal
which isn't normally mapped to any user, in which case svcgssd passes
down uid -1, which the kernel is then expected to map to the
export-specific anonymous uid or gid.
The new uid_valid/gid_valid checks were therefore causing that downcall
to fail.
(Note the regression may not have been seen with older userspace that
tended to map unknown principals to an anonymous id on their own rather
than leaving it to the kernel.)
Signed-off-by: J. Bruce Fields bfie...@redhat.com
Reviewed-by: Eric W. Biederman ebied...@xmission.com
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..c2ab26f 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /*
+ * NOTE: we don't check uid_valid() or gid_valid(): instead,
+ * -1 id's are later mapped to the (export-specific)
+ * anonymous id by nfsd_setuser.
+ *
+ * (But supplementary gid's get no such special
+ * treatment so are checked for validity here.)
+ */
/* uid */
rsci.cred.cr_uid = make_kuid(init_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(mesg, id))
goto out;
rsci.cred.cr_gid = make_kgid(init_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(mesg, N))
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
"J. Bruce Fields" writes:
> krb5 mounts started failing for me as of this patch (upstream as
> 683428fae8c73d7d7da0fa2e0b6beb4d8df4e808),
Ouch!
> and I believe the problem is
> these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
> values to indicate "authentication succeeded, but treat this user as
> anonymous". We delay mapping -1 id's to the (configurable-per-export)
> anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
>
> if (uid_eq(new->fsuid, INVALID_UID))
> new->fsuid = exp->ex_anon_uid;
> if (gid_eq(new->fsgid, INVALID_GID))
> new->fsgid = exp->ex_anon_gid;
>
> (We wouldn't be able to do that earlier because we don't know the export
> yet.)
>
> Confirmed that the following fixes the regression.
>
> Eric, any objection to removing those checks?
Not strongly. As long as we have the uid ang gid valid checks in
there somewhere before we start using these uids and gids much.
This does seems to be the case of using out of range uids and gids
to mean out of range uids and gids.
In the description of my original patch before I split it up, I
noted that one of the small dangers might be that I had added
some possibly unneceessary uid and gid valid checks. So it seems I had
even considered this slight possibility once and noted that there was a
slight chance we might have to remove a few of these.
It would be nice to have a comment to say that we expect this to happen
so people don't start wondering why there are missing checks on this
path.
There is also a gid_valid check in the secondary uids. It looks like we
should keep that as we don't have any checks for invalid gids in
nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
Also looking towards a future in which all of these things don't reside
in the initial user namespace. Is mapping any out of range uid to
INVALID_UID and then into ex_anon_uid the always the right thing to do?
Or is -1 a very special case in this instance. In which case we
probably want checks that look like:
if (-1 == id) {
rsci.cred.cr_uid = INVALID_UID;
} else {
rsci.cred.cr_uid = make_kuid(_user_ns, id);
if (!uid_valid(rsci.cred.cr_uid))
goto out;
}
Eric
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
> b/net/sunrpc/auth_gss/svcauth_gss.c
> index f7d34e7..59a089d 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -449,15 +449,11 @@ static int rsc_parse(struct cache_detail *cd,
>
> /* uid */
> rsci.cred.cr_uid = make_kuid(_user_ns, id);
> - if (!uid_valid(rsci.cred.cr_uid))
> - goto out;
>
> /* gid */
> if (get_int(, ))
> goto out;
> rsci.cred.cr_gid = make_kgid(_user_ns, id);
> - if (!gid_valid(rsci.cred.cr_gid))
> - goto out;
>
> /* number of additional gid's */
> if (get_int(, ))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
On Wed, Feb 13, 2013 at 09:51:38AM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman"
>
> For each received uid call make_kuid and validate the result.
> For each received gid call make_kgid and validate the result.
>
> Cc: "J. Bruce Fields"
> Cc: Trond Myklebust
> Signed-off-by: "Eric W. Biederman"
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 18 +-
> 1 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
> b/net/sunrpc/auth_gss/svcauth_gss.c
> index 73e9573..ecd1d58 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -418,6 +418,7 @@ static int rsc_parse(struct cache_detail *cd,
> {
> /* contexthandle expiry [ uid gid N mechname ...mechdata... ]
> */
> char *buf = mesg;
> + int id;
> int len, rv;
> struct rsc rsci, *rscp = NULL;
> time_t expiry;
> @@ -444,7 +445,7 @@ static int rsc_parse(struct cache_detail *cd,
> goto out;
>
> /* uid, or NEGATIVE */
> - rv = get_int(, _uid);
> + rv = get_int(, );
> if (rv == -EINVAL)
> goto out;
> if (rv == -ENOENT)
> @@ -452,8 +453,16 @@ static int rsc_parse(struct cache_detail *cd,
> else {
> int N, i;
>
> + /* uid */
> + rsci.cred.cr_uid = make_kuid(_user_ns, id);
> + if (!uid_valid(rsci.cred.cr_uid))
> + goto out;
> +
> /* gid */
> - if (get_int(, _gid))
> + if (get_int(, ))
> + goto out;
> + rsci.cred.cr_gid = make_kgid(_user_ns, id);
> + if (!gid_valid(rsci.cred.cr_gid))
> goto out;
krb5 mounts started failing for me as of this patch (upstream as
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808), and I believe the problem is
these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
values to indicate "authentication succeeded, but treat this user as
anonymous". We delay mapping -1 id's to the (configurable-per-export)
anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
if (uid_eq(new->fsuid, INVALID_UID))
new->fsuid = exp->ex_anon_uid;
if (gid_eq(new->fsgid, INVALID_GID))
new->fsgid = exp->ex_anon_gid;
(We wouldn't be able to do that earlier because we don't know the export
yet.)
Confirmed that the following fixes the regression.
Eric, any objection to removing those checks?
--b.
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..59a089d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -449,15 +449,11 @@ static int rsc_parse(struct cache_detail *cd,
/* uid */
rsci.cred.cr_uid = make_kuid(_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(, ))
goto out;
rsci.cred.cr_gid = make_kgid(_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(, ))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
On Wed, Feb 13, 2013 at 09:51:38AM -0800, Eric W. Biederman wrote:
From: Eric W. Biederman ebied...@xmission.com
For each received uid call make_kuid and validate the result.
For each received gid call make_kgid and validate the result.
Cc: J. Bruce Fields bfie...@fieldses.org
Cc: Trond Myklebust trond.mykleb...@netapp.com
Signed-off-by: Eric W. Biederman ebied...@xmission.com
---
net/sunrpc/auth_gss/svcauth_gss.c | 18 +-
1 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index 73e9573..ecd1d58 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -418,6 +418,7 @@ static int rsc_parse(struct cache_detail *cd,
{
/* contexthandle expiry [ uid gid N n gids mechname ...mechdata... ]
*/
char *buf = mesg;
+ int id;
int len, rv;
struct rsc rsci, *rscp = NULL;
time_t expiry;
@@ -444,7 +445,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
/* uid, or NEGATIVE */
- rv = get_int(mesg, rsci.cred.cr_uid);
+ rv = get_int(mesg, id);
if (rv == -EINVAL)
goto out;
if (rv == -ENOENT)
@@ -452,8 +453,16 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /* uid */
+ rsci.cred.cr_uid = make_kuid(init_user_ns, id);
+ if (!uid_valid(rsci.cred.cr_uid))
+ goto out;
+
/* gid */
- if (get_int(mesg, rsci.cred.cr_gid))
+ if (get_int(mesg, id))
+ goto out;
+ rsci.cred.cr_gid = make_kgid(init_user_ns, id);
+ if (!gid_valid(rsci.cred.cr_gid))
goto out;
krb5 mounts started failing for me as of this patch (upstream as
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808), and I believe the problem is
these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
values to indicate authentication succeeded, but treat this user as
anonymous. We delay mapping -1 id's to the (configurable-per-export)
anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
if (uid_eq(new-fsuid, INVALID_UID))
new-fsuid = exp-ex_anon_uid;
if (gid_eq(new-fsgid, INVALID_GID))
new-fsgid = exp-ex_anon_gid;
(We wouldn't be able to do that earlier because we don't know the export
yet.)
Confirmed that the following fixes the regression.
Eric, any objection to removing those checks?
--b.
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..59a089d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -449,15 +449,11 @@ static int rsc_parse(struct cache_detail *cd,
/* uid */
rsci.cred.cr_uid = make_kuid(init_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(mesg, id))
goto out;
rsci.cred.cr_gid = make_kgid(init_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(mesg, N))
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
J. Bruce Fields bfie...@fieldses.org writes:
krb5 mounts started failing for me as of this patch (upstream as
683428fae8c73d7d7da0fa2e0b6beb4d8df4e808),
Ouch!
and I believe the problem is
these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
values to indicate authentication succeeded, but treat this user as
anonymous. We delay mapping -1 id's to the (configurable-per-export)
anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
if (uid_eq(new-fsuid, INVALID_UID))
new-fsuid = exp-ex_anon_uid;
if (gid_eq(new-fsgid, INVALID_GID))
new-fsgid = exp-ex_anon_gid;
(We wouldn't be able to do that earlier because we don't know the export
yet.)
Confirmed that the following fixes the regression.
Eric, any objection to removing those checks?
Not strongly. As long as we have the uid ang gid valid checks in
there somewhere before we start using these uids and gids much.
This does seems to be the case of using out of range uids and gids
to mean out of range uids and gids.
In the description of my original patch before I split it up, I
noted that one of the small dangers might be that I had added
some possibly unneceessary uid and gid valid checks. So it seems I had
even considered this slight possibility once and noted that there was a
slight chance we might have to remove a few of these.
It would be nice to have a comment to say that we expect this to happen
so people don't start wondering why there are missing checks on this
path.
There is also a gid_valid check in the secondary uids. It looks like we
should keep that as we don't have any checks for invalid gids in
nfsd_setuser. Is that possibly an oversight in nfsd_setuser?
Also looking towards a future in which all of these things don't reside
in the initial user namespace. Is mapping any out of range uid to
INVALID_UID and then into ex_anon_uid the always the right thing to do?
Or is -1 a very special case in this instance. In which case we
probably want checks that look like:
if (-1 == id) {
rsci.cred.cr_uid = INVALID_UID;
} else {
rsci.cred.cr_uid = make_kuid(init_user_ns, id);
if (!uid_valid(rsci.cred.cr_uid))
goto out;
}
Eric
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..59a089d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -449,15 +449,11 @@ static int rsc_parse(struct cache_detail *cd,
/* uid */
rsci.cred.cr_uid = make_kuid(init_user_ns, id);
- if (!uid_valid(rsci.cred.cr_uid))
- goto out;
/* gid */
if (get_int(mesg, id))
goto out;
rsci.cred.cr_gid = make_kgid(init_user_ns, id);
- if (!gid_valid(rsci.cred.cr_gid))
- goto out;
/* number of additional gid's */
if (get_int(mesg, N))
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
From: "Eric W. Biederman"
For each received uid call make_kuid and validate the result.
For each received gid call make_kgid and validate the result.
Cc: "J. Bruce Fields"
Cc: Trond Myklebust
Signed-off-by: "Eric W. Biederman"
---
net/sunrpc/auth_gss/svcauth_gss.c | 18 +-
1 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index 73e9573..ecd1d58 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -418,6 +418,7 @@ static int rsc_parse(struct cache_detail *cd,
{
/* contexthandle expiry [ uid gid N mechname ...mechdata... ]
*/
char *buf = mesg;
+ int id;
int len, rv;
struct rsc rsci, *rscp = NULL;
time_t expiry;
@@ -444,7 +445,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
/* uid, or NEGATIVE */
- rv = get_int(, _uid);
+ rv = get_int(, );
if (rv == -EINVAL)
goto out;
if (rv == -ENOENT)
@@ -452,8 +453,16 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /* uid */
+ rsci.cred.cr_uid = make_kuid(_user_ns, id);
+ if (!uid_valid(rsci.cred.cr_uid))
+ goto out;
+
/* gid */
- if (get_int(, _gid))
+ if (get_int(, ))
+ goto out;
+ rsci.cred.cr_gid = make_kgid(_user_ns, id);
+ if (!gid_valid(rsci.cred.cr_gid))
goto out;
/* number of additional gid's */
@@ -467,11 +476,10 @@ static int rsc_parse(struct cache_detail *cd,
/* gid's */
status = -EINVAL;
for (i=0; ihttp://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec_contect cache
From: Eric W. Biederman ebied...@xmission.com
For each received uid call make_kuid and validate the result.
For each received gid call make_kgid and validate the result.
Cc: J. Bruce Fields bfie...@fieldses.org
Cc: Trond Myklebust trond.mykleb...@netapp.com
Signed-off-by: Eric W. Biederman ebied...@xmission.com
---
net/sunrpc/auth_gss/svcauth_gss.c | 18 +-
1 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
b/net/sunrpc/auth_gss/svcauth_gss.c
index 73e9573..ecd1d58 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -418,6 +418,7 @@ static int rsc_parse(struct cache_detail *cd,
{
/* contexthandle expiry [ uid gid N n gids mechname ...mechdata... ]
*/
char *buf = mesg;
+ int id;
int len, rv;
struct rsc rsci, *rscp = NULL;
time_t expiry;
@@ -444,7 +445,7 @@ static int rsc_parse(struct cache_detail *cd,
goto out;
/* uid, or NEGATIVE */
- rv = get_int(mesg, rsci.cred.cr_uid);
+ rv = get_int(mesg, id);
if (rv == -EINVAL)
goto out;
if (rv == -ENOENT)
@@ -452,8 +453,16 @@ static int rsc_parse(struct cache_detail *cd,
else {
int N, i;
+ /* uid */
+ rsci.cred.cr_uid = make_kuid(init_user_ns, id);
+ if (!uid_valid(rsci.cred.cr_uid))
+ goto out;
+
/* gid */
- if (get_int(mesg, rsci.cred.cr_gid))
+ if (get_int(mesg, id))
+ goto out;
+ rsci.cred.cr_gid = make_kgid(init_user_ns, id);
+ if (!gid_valid(rsci.cred.cr_gid))
goto out;
/* number of additional gid's */
@@ -467,11 +476,10 @@ static int rsc_parse(struct cache_detail *cd,
/* gid's */
status = -EINVAL;
for (i=0; iN; i++) {
- gid_t gid;
kgid_t kgid;
- if (get_int(mesg, gid))
+ if (get_int(mesg, id))
goto out;
- kgid = make_kgid(init_user_ns, gid);
+ kgid = make_kgid(init_user_ns, id);
if (!gid_valid(kgid))
goto out;
GROUP_AT(rsci.cred.cr_group_info, i) = kgid;
--
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
