On Tue, Mar 20, 2018 at 7:27 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: > Miklos Szeredi <mszeredi@xxxxxxxxxx> writes: >> I did just one modification to "fuse: Fail all requests with invalid >> uids or gids": instead of zeroing out the context for the nofail case, >> continue to use the "_munged" variants. I don't think this hurts and >> is better for backward compatibility (I guess the only relevant use >> would be for debugging output, but we don't want to regress even for >> that if not necessary) > > Hmm... > > The thing is the failure doesn't come in the difference between the > _munged and the normal variants. The difference between > munged and non-munged variants is how they handled failure ((uid16_t)-2) > aka 0xfffe for munged and -1 for the non-munged case. > > The failures are introduced by changing &init_user_ns to fc->user_ns. Right. > The operations in question are iop->flush and fuse_force_forget (on an > error). I don't know what value having ids on those paths will do > they are operations that must succeed, and they should not change the > on-disk ids. I was thinking saying the most privileged id was asking > for the oepration would seem to make sense. I don't think anybody should actually *care* about the id's in flush, but I'd still not change the current behavior for change's sake. > > With the munged variants we will get (uid16_t)-2 aka 0xfffe aka > nobody asking for the operation if things don't map. In practice > the don't map case is new. > > Since the id's should not be looked at anyway I don't see it makes > much difference which ids we use so the munged case seems at least > plausible. > > It might be better to use the non-munghed variant and do: > if (req->in.h.uid == (uid_t)-1) > req.in.h.uid = 0; > if (req->in.h.gid == (gid_t)-1) > req.in.h.gid = 0; > > That might be less surprising to userspace. As I don't think the > unmapped case has ever occurred in practice yet. Right, that would work too, but I don't think it actually matters, so unless you can think of an actual security issue arising from using the munged variants, I'd just leave it as it is. Thanks, Miklos
