On Thu, Jul 16, 2020 at 04:39:14PM +0200, Mickaël Salaün wrote:

On 15/07/2020 22:37, Kees Cook wrote:

quoted

On Tue, Jul 14, 2020 at 08:16:36PM +0200, Mickaël Salaün wrote:

quoted

@@ -2849,7 +2855,7 @@ static int may_open(const struct path *path, int acc_mode, int flag)
 	case S_IFLNK:
 		return -ELOOP;
 	case S_IFDIR:
-		if (acc_mode & (MAY_WRITE | MAY_EXEC))
+		if (acc_mode & (MAY_WRITE | MAY_EXEC | MAY_OPENEXEC))
 			return -EISDIR;
 		break;

(I need to figure out where "open for reading" rejects S_IFDIR, since
it's clearly not here...)

Doesn't it come from generic_read_dir() in fs/libfs.c?

quoted

quoted

 	case S_IFBLK:

@@ -2859,13 +2865,26 @@ static int may_open(const struct path *path, int acc_mode, int flag)
 		fallthrough;
 	case S_IFIFO:
 	case S_IFSOCK:
-		if (acc_mode & MAY_EXEC)
+		if (acc_mode & (MAY_EXEC | MAY_OPENEXEC))
 			return -EACCES;
 		flag &= ~O_TRUNC;
 		break;

This will immediately break a system that runs code with MAY_OPENEXEC
set but reads from a block, char, fifo, or socket, even in the case of
a sysadmin leaving the "file" sysctl disabled.

As documented, O_MAYEXEC is for regular files. The only legitimate use
case seems to be with pipes, which should probably be allowed when
enforcement is disabled.

By the way Kees, while we fix that for the next series, do you think it
would be relevant, at least for the sake of clarity, to add a
WARN_ON_ONCE(acc_mode & MAY_OPENEXEC) for the S_IFSOCK case, since a
socket cannot be open anyway?

-- 
Thibaut Sautereau
CLIP OS developer