On 09/03/18 05:11, Ben Crocker wrote: > Sent that a little too soon; please consider "Reviewed-by: Ben Crocker > <[email protected] <mailto:[email protected]>>" to be > added after each of the "Signed-off-by: Nicolai Haehnle > <[email protected] <mailto:[email protected]>>" lines. You can add: Reviewed-by: Antoine Martin <[email protected]>
You should probably re-send the patch series as individual emails. Cheers Antoine > > Also please note that I rebased Nicolai's original patch, > https://patchwork.freedesktop.org/series/18684/, > against a very recent copy of master. > > Thanks, > Ben > > > On Thu, Mar 8, 2018 at 4:53 PM, Ben Crocker <[email protected] > <mailto:[email protected]>> wrote: > > From: Nicolai Hähnle <[email protected] <mailto:[email protected]>> > > From: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > > Having different types of code all trying to check for elevated > privileges > is a bad idea. This implementation is the most thorough one. > > Signed-off-by: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > --- > hw/xfree86/common/xf86Init.c | 59 > +---------------------------------------- > include/os.h | 3 +++ > os/utils.c | 63 > ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 67 insertions(+), 58 deletions(-) > > diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c > index e61fe66..758b926 100644 > --- a/hw/xfree86/common/xf86Init.c > +++ b/hw/xfree86/common/xf86Init.c > @@ -230,78 +230,21 @@ xf86PrintBanner(void) > xf86ErrorFVerb(0, "Current version of pixman: %s\n", > pixman_version_string()); > xf86ErrorFVerb(0, "\tBefore reporting problems, check " > "" __VENDORDWEBSUPPORT__ "\n" > "\tto make sure that you have the latest > version.\n"); > } > > Bool > xf86PrivsElevated(void) > { > - static Bool privsTested = FALSE; > - static Bool privsElevated = TRUE; > - > - if (!privsTested) { > -#if defined(WIN32) > - privsElevated = FALSE; > -#else > - if ((getuid() != geteuid()) || (getgid() != getegid())) { > - privsElevated = TRUE; > - } > - else { > -#if defined(HAVE_ISSETUGID) > - privsElevated = issetugid(); > -#elif defined(HAVE_GETRESUID) > - uid_t ruid, euid, suid; > - gid_t rgid, egid, sgid; > - > - if ((getresuid(&ruid, &euid, &suid) == 0) && > - (getresgid(&rgid, &egid, &sgid) == 0)) { > - privsElevated = (euid != suid) || (egid != sgid); > - } > - else { > - printf("Failed getresuid or getresgid"); > - /* Something went wrong, make defensive assumption */ > - privsElevated = TRUE; > - } > -#else > - if (getuid() == 0) { > - /* running as root: uid==euid==0 */ > - privsElevated = FALSE; > - } > - else { > - /* > - * If there are saved ID's the process might still > be privileged > - * even though the above test succeeded. If > issetugid() and > - * getresgid() aren't available, test this by > trying to set > - * euid to 0. > - */ > - unsigned int oldeuid; > - > - oldeuid = geteuid(); > - > - if (seteuid(0) != 0) { > - privsElevated = FALSE; > - } > - else { > - if (seteuid(oldeuid) != 0) { > - FatalError("Failed to drop privileges. > Exiting\n"); > - } > - privsElevated = TRUE; > - } > - } > -#endif > - } > -#endif > - privsTested = TRUE; > - } > - return privsElevated; > + return PrivsElevated(); > } > > static void > TrapSignals(void) > { > if (xf86Info.notrapSignals) { > OsSignal(SIGSEGV, SIG_DFL); > OsSignal(SIGABRT, SIG_DFL); > OsSignal(SIGILL, SIG_DFL); > #ifdef SIGEMT > diff --git a/include/os.h b/include/os.h > index d2c41b4..686f6d6 100644 > --- a/include/os.h > +++ b/include/os.h > @@ -355,20 +355,23 @@ Fclose(void *); > extern const char * > Win32TempDir(void); > > extern int > System(const char *cmdline); > > #define Fopen(a,b) fopen(a,b) > #define Fclose(a) fclose(a) > #endif > > +extern _X_EXPORT Bool > +PrivsElevated(void); > + > extern _X_EXPORT void > CheckUserParameters(int argc, char **argv, char **envp); > extern _X_EXPORT void > CheckUserAuthorization(void); > > extern _X_EXPORT int > AddHost(ClientPtr /*client */ , > int /*family */ , > unsigned /*length */ , > const void * /*pAddr */ ); > diff --git a/os/utils.c b/os/utils.c > index ac55cd7..024989e 100644 > --- a/os/utils.c > +++ b/os/utils.c > @@ -1717,20 +1717,83 @@ System(const char *cmdline) > > /* Close process and thread handles. */ > CloseHandle(pi.hProcess); > CloseHandle(pi.hThread); > free(cmd); > > return dwExitCode; > } > #endif > > +Bool > +PrivsElevated(void) > +{ > + static Bool privsTested = FALSE; > + static Bool privsElevated = TRUE; > + > + if (!privsTested) { > +#if defined(WIN32) > + privsElevated = FALSE; > +#else > + if ((getuid() != geteuid()) || (getgid() != getegid())) { > + privsElevated = TRUE; > + } > + else { > +#if defined(HAVE_ISSETUGID) > + privsElevated = issetugid(); > +#elif defined(HAVE_GETRESUID) > + uid_t ruid, euid, suid; > + gid_t rgid, egid, sgid; > + > + if ((getresuid(&ruid, &euid, &suid) == 0) && > + (getresgid(&rgid, &egid, &sgid) == 0)) { > + privsElevated = (euid != suid) || (egid != sgid); > + } > + else { > + printf("Failed getresuid or getresgid"); > + /* Something went wrong, make defensive assumption */ > + privsElevated = TRUE; > + } > +#else > + if (getuid() == 0) { > + /* running as root: uid==euid==0 */ > + privsElevated = FALSE; > + } > + else { > + /* > + * If there are saved ID's the process might still > be privileged > + * even though the above test succeeded. If > issetugid() and > + * getresgid() aren't available, test this by > trying to set > + * euid to 0. > + */ > + unsigned int oldeuid; > + > + oldeuid = geteuid(); > + > + if (seteuid(0) != 0) { > + privsElevated = FALSE; > + } > + else { > + if (seteuid(oldeuid) != 0) { > + FatalError("Failed to drop privileges. > Exiting\n"); > + } > + privsElevated = TRUE; > + } > + } > +#endif > + } > +#endif > + privsTested = TRUE; > + } > + return privsElevated; > +} > + > /* > * CheckUserParameters: check for long command line arguments and long > * environment variables. By default, these checks are only done when > * the server's euid != ruid. In 3.3.x, these checks were done in an > * external wrapper utility. > */ > > /* Consider LD* variables insecure? */ > #ifndef REMOVE_ENV_LD > #define REMOVE_ENV_LD 1 > > From patchwork Fri Jan 27 13:37:36 2017 > Content-Type: text/plain; charset="utf-8" > MIME-Version: 1.0 > Content-Transfer-Encoding: 8bit > Subject: [xserver,2/4] os: use PrivsElevated instead of a manual check > From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > X-Patchwork-Id: 135711 > Message-Id: <[email protected] > <mailto:[email protected]>> > To: [email protected] <mailto:[email protected]> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > Date: Fri, 27 Jan 2017 14:37:36 +0100 > > From: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > > Signed-off-by: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > --- > os/utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/os/utils.c b/os/utils.c > index 024989e..05733b0 100644 > --- a/os/utils.c > +++ b/os/utils.c > @@ -1861,21 +1861,21 @@ enum BadCode { > #endif > > void > CheckUserParameters(int argc, char **argv, char **envp) > { > enum BadCode bad = NotBad; > int i = 0, j; > char *a, *e = NULL; > > #if CHECK_EUID > - if (geteuid() == 0 && getuid() != geteuid()) > + if (PrivsElevated()) > #endif > { > /* Check each argv[] */ > for (i = 1; i < argc; i++) { > if (strcmp(argv[i], "-fp") == 0) { > i++; /* continue with next argument. > skip the length check */ > if (i >= argc) > break; > } > else { > > From patchwork Fri Jan 27 13:37:37 2017 > Content-Type: text/plain; charset="utf-8" > MIME-Version: 1.0 > Content-Transfer-Encoding: 8bit > Subject: [xserver, 3/4] xfree86: replace all uses of > xf86PrivsElevated with > PrivsElevated > From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > X-Patchwork-Id: 135712 > Message-Id: <[email protected] > <mailto:[email protected]>> > To: [email protected] <mailto:[email protected]> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > Date: Fri, 27 Jan 2017 14:37:37 +0100 > > From: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > > Signed-off-by: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > --- > hw/xfree86/common/xf86Config.c | 2 +- > hw/xfree86/common/xf86Init.c | 12 +++--------- > hw/xfree86/common/xf86Priv.h | 2 -- > 3 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/hw/xfree86/common/xf86Config.c > b/hw/xfree86/common/xf86Config.c > index f03acf3..5d2cf0a 100644 > --- a/hw/xfree86/common/xf86Config.c > +++ b/hw/xfree86/common/xf86Config.c > @@ -2309,21 +2309,21 @@ xf86HandleConfigFile(Bool autoconfig) > #endif > Bool implicit_layout = FALSE; > XF86ConfLayoutPtr layout; > > if (!autoconfig) { > char *filename, *dirname, *sysdirname; > const char *filesearch, *dirsearch; > MessageType filefrom = X_DEFAULT; > MessageType dirfrom = X_DEFAULT; > > - if (!xf86PrivsElevated()) { > + if (!PrivsElevated()) { > filesearch = ALL_CONFIGPATH; > dirsearch = ALL_CONFIGDIRPATH; > } > else { > filesearch = RESTRICTED_CONFIGPATH; > dirsearch = RESTRICTED_CONFIGDIRPATH; > } > > if (xf86ConfigFile) > filefrom = X_CMDLINE; > diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c > index 758b926..deed91f 100644 > --- a/hw/xfree86/common/xf86Init.c > +++ b/hw/xfree86/common/xf86Init.c > @@ -227,26 +227,20 @@ xf86PrintBanner(void) > #if defined(BUILDERSTRING) > xf86ErrorFVerb(0, "%s \n", BUILDERSTRING); > #endif > xf86ErrorFVerb(0, "Current version of pixman: %s\n", > pixman_version_string()); > xf86ErrorFVerb(0, "\tBefore reporting problems, check " > "" __VENDORDWEBSUPPORT__ "\n" > "\tto make sure that you have the latest > version.\n"); > } > > -Bool > -xf86PrivsElevated(void) > -{ > - return PrivsElevated(); > -} > - > static void > TrapSignals(void) > { > if (xf86Info.notrapSignals) { > OsSignal(SIGSEGV, SIG_DFL); > OsSignal(SIGABRT, SIG_DFL); > OsSignal(SIGILL, SIG_DFL); > #ifdef SIGEMT > OsSignal(SIGEMT, SIG_DFL); > #endif > @@ -886,21 +880,21 @@ OsVendorInit(void) > /* Set stderr to non-blocking. */ > #ifndef O_NONBLOCK > #if defined(FNDELAY) > #define O_NONBLOCK FNDELAY > #elif defined(O_NDELAY) > #define O_NONBLOCK O_NDELAY > #endif > > #ifdef O_NONBLOCK > if (!beenHere) { > - if (xf86PrivsElevated()) { > + if (PrivsElevated()) { > int status; > > status = fcntl(fileno(stderr), F_GETFL, 0); > if (status != -1) { > fcntl(fileno(stderr), F_SETFL, status | O_NONBLOCK); > } > } > } > #endif > #endif > @@ -1041,21 +1035,21 @@ xf86PrintDefaultModulePath(void) > > static void > xf86PrintDefaultLibraryPath(void) > { > ErrorF("%s\n", DEFAULT_LIBRARY_PATH); > } > > static void > xf86CheckPrivs(const char *option, const char *arg) > { > - if (xf86PrivsElevated() && !xf86PathIsSafe(arg)) { > + if (PrivsElevated() && !xf86PathIsSafe(arg)) { > FatalError("\nInvalid argument for %s - \"%s\"\n" > "\tWith elevated privileges %s must specify a > relative path\n" > "\twithout any \"..\" elements.\n\n", option, > arg, option); > } > } > > /* > * ddxProcessArgument -- > * Process device-dependent command line args. Returns 0 if > argument is > * not device dependent, otherwise Count of number of elements > of argv > @@ -1342,21 +1336,21 @@ ddxProcessArgument(int argc, char **argv, int i) > * Print out correct use of device dependent commandline options. > * Maybe the user now knows what really to do ... > */ > > void > ddxUseMsg(void) > { > ErrorF("\n"); > ErrorF("\n"); > ErrorF("Device Dependent Usage\n"); > - if (!xf86PrivsElevated()) { > + if (!PrivsElevated()) { > ErrorF("-modulepath paths specify the module search > path\n"); > ErrorF("-logfile file specify a log file name\n"); > ErrorF("-configure probe for devices and write an " > XCONFIGFILE "\n"); > ErrorF > ("-showopts print available options for > all installed drivers\n"); > } > ErrorF > ("-config file specify a configuration file, > relative to the\n"); > ErrorF(" " XCONFIGFILE > diff --git a/hw/xfree86/common/xf86Priv.h b/hw/xfree86/common/xf86Priv.h > index c1f8a18..d78e8f6 100644 > --- a/hw/xfree86/common/xf86Priv.h > +++ b/hw/xfree86/common/xf86Priv.h > @@ -154,16 +154,14 @@ xf86CloseLog(enum ExitCode error); > > /* xf86Init.c */ > extern _X_EXPORT Bool > xf86LoadModules(const char **list, void **optlist); > extern _X_EXPORT int > xf86SetVerbosity(int verb); > extern _X_EXPORT int > xf86SetLogVerbosity(int verb); > extern _X_EXPORT Bool > xf86CallDriverProbe(struct _DriverRec *drv, Bool detect_only); > -extern _X_EXPORT Bool > -xf86PrivsElevated(void); > > #endif /* _NO_XF86_PROTOTYPES */ > > #endif /* _XF86PRIV_H */ > > From patchwork Fri Jan 27 13:37:38 2017 > Content-Type: text/plain; charset="utf-8" > MIME-Version: 1.0 > Content-Transfer-Encoding: 8bit > Subject: [xserver,4/4] glx: honor LIBGL_DRIVERS_PATH when loading > DRI drivers > From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > X-Patchwork-Id: 135713 > Message-Id: <[email protected] > <mailto:[email protected]>> > To: [email protected] <mailto:[email protected]> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <[email protected] > <mailto:[email protected]>> > Date: Fri, 27 Jan 2017 14:37:38 +0100 > > From: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > > Allow switching to another driver build without a full installation. > > Glamor already takes LIBGL_DRIVERS_PATH into account, so this change > makes sure that the same driver is used in both parts of the server. > > Signed-off-by: Nicolai Hähnle <[email protected] > <mailto:[email protected]>> > --- > glx/glxdricommon.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/glx/glxdricommon.c b/glx/glxdricommon.c > index f6c6fcd..a370845 100644 > --- a/glx/glxdricommon.c > +++ b/glx/glxdricommon.c > @@ -241,28 +241,58 @@ static const char dri_driver_path[] = > DRI_DRIVER_PATH; > void * > glxProbeDriver(const char *driverName, > void **coreExt, const char *coreName, int coreVersion, > void **renderExt, const char *renderName, int > renderVersion) > { > int i; > void *driver; > char filename[PATH_MAX]; > char *get_extensions_name; > const __DRIextension **extensions = NULL; > + const char *path = NULL; > + > + /* Search in LIBGL_DRIVERS_PATH if we're not setuid. */ > + if (!PrivsElevated()) > + path = getenv("LIBGL_DRIVERS_PATH"); > + > + if (!path) > + path = dri_driver_path; > + > + do { > + const char *next; > + int path_len; > + > + next = strchr(path, ':'); > + if (next) { > + path_len = next - path; > + next++; > + } else { > + path_len = strlen(path); > + next = NULL; > + } > > - snprintf(filename, sizeof filename, "%s/%s_dri.so", > - dri_driver_path, driverName); > + snprintf(filename, sizeof filename, "%.*s/%s_dri.so", > path_len, path, > + driverName); > + > + driver = dlopen(filename, RTLD_LAZY | RTLD_LOCAL); > + if (driver != NULL) > + break; > > - driver = dlopen(filename, RTLD_LAZY | RTLD_LOCAL); > - if (driver == NULL) { > LogMessage(X_ERROR, "AIGLX error: dlopen of %s failed (%s)\n", > filename, dlerror()); > + > + path = next; > + } while (path); > + > + if (driver == NULL) { > + LogMessage(X_ERROR, "AIGLX error: unable to load driver %s\n", > + driverName); > goto cleanup_failure; > } > > if (asprintf(&get_extensions_name, "%s_%s", > __DRI_DRIVER_GET_EXTENSIONS, driverName) != -1) { > const __DRIextension **(*get_extensions)(void); > > get_extensions = dlsym(driver, get_extensions_name); > if (get_extensions) > extensions = get_extensions(); > > > > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
