Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Tetsuo Handa]

On 2024/01/25 3:27, Linus Torvalds wrote:
> The whole cred use of current->in_execve in tomoyo should
> *also* be fixed, but I didn't even try to follow what it actually
> wanted.

Due to TOMOYO's unique domain transition (transits to new domain before
execve() succeeds and returns to old domain if execve() failed), TOMOYO
depends on a tricky ordering shown below.

----------
// a caller tries execve().
sys_execve() {
  do_execve() {
    do_execveat_common() {
      bprm_execve() {
        prepare_bprm_creds() {
          prepare_exec_creds() {
            prepare_creds() {
              security_prepare_creds() {
                tomoyo_cred_prepare() {
                  if (s->old_domain_info && !current->in_execve) { // false because s->old_domain_info == NULL.
                    s->domain_info = s->old_domain_info;
                    s->old_domain_info = NULL; 
                  }
                }
              }
            }
          }
        }
        current->in_execve = 1;
        do_open_execat() {
          (...snipped...)
          security_file_open() {
            tomoyo_file_open() // Not checked because current->in_execve == 1.
          }
          (...snipped...)
        }
        exec_binprm() {
          search_binary_handler() {
            security_bprm_check() {
              tomoyo_bprm_check_security() {
                if (!s->old_domain_info) { // true because s->old_domain_info == NULL.
                  tomoyo_find_next_domain() {
                    // Checks execute permission here.
                    s->old_domain_info = s->domain_info; // Remember old domain.
                    s->domain_info = domain; // Transit to new domain.
                  }
                }
              }
            }
            fmt->load_binary() { // e.g. load_script() in fs/binfmt_script.c
              open_exec() {
                // Not checked because current->in_execve == 1.
              }
            }
          }
          search_binary_handler() {
            security_bprm_check() {
              tomoyo_bprm_check_security() {
                if (!s->old_domain_info) { // false because s->old_domain_info != NULL.
                } else {
                  // Checks read permission here.
                }
              }
            }
          }
          // An error happens after s->domain_info was updated.
        }
        current->in_execve = 0;
        // No chance to restore s->domain_info.
      }
    }
  }
  // returning an error code to the caller.
}
// the caller retries execve().
sys_execve() {
  do_execve() {
    do_execveat_common() {
      bprm_execve() {
        prepare_bprm_creds() {
          prepare_exec_creds() {
            prepare_creds() {
              security_prepare_creds() {
                tomoyo_cred_prepare() {
                  if (s->old_domain_info && !current->in_execve) { // true because s->old_domain_info != NULL && current->in_execve == 0.
                    s->domain_info = s->old_domain_info; // Transit to old domain.
                    s->old_domain_info = NULL;
                  }
                }
              }
            }
          }
        }
        current->in_execve = 1;
        do_open_execat() {
          (...snipped...)
          security_file_open() {
            tomoyo_file_open() // Not checked because current->in_execve == 1.
          }
          (...snipped...)
        }
        exec_binprm() {
          search_binary_handler() {
            security_bprm_check() {
              tomoyo_bprm_check_security() {
                if (!s->old_domain_info) { // true because s->old_domain_info == NULL.
                  tomoyo_find_next_domain() {
                    // Checks execute permission here.
                    s->old_domain_info = s->domain_info; // Remember old domain.
                    s->domain_info = domain; // Transit to new domain.
                  }
                }
              }
            }
            fmt->load_binary() { // e.g. load_script() in fs/binfmt_script.c
              open_exec() {
                // Not checked because current->in_execve == 1.
              }
            }
          }
          search_binary_handler() {
            security_bprm_check() {
              tomoyo_bprm_check_security() {
                if (!s->old_domain_info) { // false because s->old_domain_info != NULL.
                } else {
                  // Checks read permission here.
                }
              }
            }
          }
          fmt->load_binary() { // e.g. load_elf_binary() in fs/binfmt_elf.c
            begin_new_exec() {
              security_bprm_committed_creds() {
                tomoyo_bprm_committed_creds() {
                  s->old_domain_info = NULL; // Forget old domain.
                }
              }
            }
          }
        }
        current->in_execve = 0;
      }
    }
  }
}
----------

Commit 978ffcbf00d8 ("execve: open the executable file before doing anything else")
broke the ordering and commit 4759ff71f23e ("exec: Check __FMODE_EXEC instead of
in_execve for LSMs") and commit 3eab830189d9 ("uselib: remove use of __FMODE_EXEC")
fixed the regression.

But current->in_execve remains required unless an LSM callback that is called when
an execve() request failed which existed as security_bprm_free() until Linux 2.6.28
revives...