Re: [PATCH 2/3] mount-fstab: clean up gcc warnings

[Karel Zak <kzak@xxxxxxxxxx>]

On Wed, Oct 03, 2007 at 02:15:18PM -0700, Randy Dunlap wrote:
> From: Randy Dunlap <randy.dunlap@xxxxxxxxxx>
> 
> Fix strict gcc warnings in tailf that come from using:
>   ("-Wall -Wp,-D_FORTIFY_SOURCE=2")
> 
> fstab.c:770: warning: ignoring return value of 'chown', declared with attribute warn_unused_result

 Thanks. Applied a little different version (see below), we really
 shouldn't ignore return values from chmod/chown.

    Karel


commit 4449e12f5a4402f399eb0309ec0ed23dc09be17a
Author: Karel Zak <kzak@xxxxxxxxxx>
Date:   Wed Oct 3 14:15:18 2007 -0700

    mount: improve chmod & chown usage and clean up gcc warnings (fstab.c)
    
    Fix strict gcc warnings in tailf that come from using:
      ("-Wall -Wp,-D_FORTIFY_SOURCE=2")
    
    fstab.c:770: warning: ignoring return value of 'chown', declared with attribute warn_unused_result
    
    The patch makes chmod() and chown() mandatory. We cannot rename()
    temporary mtab to the final mtab when owner is not the same user as
    owner of the original mtab. It's security risk.
    
    Signed-off-by: Randy Dunlap <randy.dunlap@xxxxxxxxxx>
    Signed-off-by: Karel Zak <kzak@xxxxxxxxxx>

diff --git a/mount/fstab.c b/mount/fstab.c
index 0e00fc2..694f4a6 100644
--- a/mount/fstab.c
+++ b/mount/fstab.c
@@ -670,6 +670,8 @@ update_mtab (const char *dir, struct my_mntent *instead) {
 	const char *fnam = MOUNTED;
 	struct mntentchn mtabhead;	/* dummy */
 	struct mntentchn *mc, *mc0, *absent = NULL;
+	struct stat sbuf;
+	int fd;
 
 	if (mtab_does_not_exist() || !mtab_is_writable())
 		return;
@@ -751,25 +753,39 @@ update_mtab (const char *dir, struct my_mntent *instead) {
 	}
 
 	discard_mntentchn(mc0);
+	fd = fileno(mftmp->mntent_fp);
+
+	/*
+	 * It seems that better is incomplete and broken /mnt/mtab that
+	 * /mnt/mtab that is writeable for non-root users.
+	 *
+	 * We always skip rename() when chown() and chmod() failed.
+	 * -- kzak, 11-Oct-2007
+	 */
 
-	if (fchmod (fileno (mftmp->mntent_fp),
-		    S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) < 0) {
+	if (fchmod(fd, S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) < 0) {
 		int errsv = errno;
 		fprintf(stderr, _("error changing mode of %s: %s\n"),
 			MOUNTED_TEMP, strerror (errsv));
+		goto leave;
 	}
-	my_endmntent (mftmp);
 
-	{ /*
-	   * If mount is setuid and some non-root user mounts sth,
-	   * then mtab.tmp might get the group of this user. Copy uid/gid
-	   * from the present mtab before renaming.
-	   */
-	    struct stat sbuf;
-	    if (stat (MOUNTED, &sbuf) == 0)
-		chown (MOUNTED_TEMP, sbuf.st_uid, sbuf.st_gid);
+	/*
+	 * If mount is setuid and some non-root user mounts sth,
+	 * then mtab.tmp might get the group of this user. Copy uid/gid
+	 * from the present mtab before renaming.
+	 */
+	if (stat(MOUNTED, &sbuf) == 0) {
+		if (fchown(fd, sbuf.st_uid, sbuf.st_gid) < 0) {
+			int errsv = errno;
+			fprintf (stderr, _("error changing owner of %s: %s\n"),
+				MOUNTED_TEMP, strerror(errsv));
+			goto leave;
+		}
 	}
 
+	my_endmntent (mftmp);
+
 	/* rename mtemp to mtab */
 	if (rename (MOUNTED_TEMP, MOUNTED) < 0) {
 		int errsv = errno;

-- 
 Karel Zak  <kzak@xxxxxxxxxx>
-
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html