Re: CVS commit: src

[David Holland <dholland%netbsd.org@localhost>]

On Sun, Nov 18, 2012 at 05:41:55PM +0000, Emmanuel Dreyfus wrote:
 > Also implement O_SEARCH for openat(2)

What is this logic supposed to do?

+               if (!(dfp->f_flag & FSEARCH)) {
+                       error = VOP_ACCESS(dfp->f_data, VEXEC, l->l_cred);
+                       if (error)
+                               goto cleanup;
+               }

It appears three times, and in all three cases it:

   (1) is highly suspect because it bypasses a security check if
       FSEARCH (aka O_SEARCH) is set, but no security check that I can
       find is required to set FSEARCH;

   (2) is not atomic and is therefore just as useless as calling
       access(2) for security checking;

   (3) violates the vnode locking protocol;

   (4) is fortunately unexploitable on these points because it is
       redundant and therefore useless.

Also, the semantics of these new O_* flags remains under discussion in
tech-kern, as they seem to be suspect (not unrelated to point 1) so
at least the O_SEARCH part of this commit seems rather premature.

(Did you post the O_SEARCH changes for review? If so, I must have
missed them.)

In any event please revert at least the O_SEARCH changes. Once the
discussion of the semantics is finished, please post a new patch that
implements the conclusion of the discussion and doesn't suffer from
the points noted above.

thanks.

-- 
David A. Holland
dholland%netbsd.org@localhost