linux-kernel - Re: [PATCH] utimensat() non-conformances and fixes [v3]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <E1K2ABK-0002ck-UT@pomaz-ex.szeredi.hu>
Date:	Fri, 30 May 2008 21:22:30 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	mtk.manpages@...glemail.com
CC:	miklos@...redi.hu, drepper@...hat.com, viro@...iv.linux.org.uk,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-man@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] utimensat() non-conformances and fixes [v3]

> >> Here's a further version of the patch, against 2.6.26rc2, with the
> >> 2008-05-19 git changes you sent me applied.  This patch is based on
> >> the draft patch you sent me.  I've tested this version of the patch,
> >> and it works as for all cases except the one mentioned below.  But
> >> note the following points:
> >>
> >> 1) I didn't make use of the code in notify_change() that checks
> >> IS_IMMUTABLE() and IS_APPEND() (i.e., I did not add
> >> ATTR_OWNER_CHECK to the mask in the controlling if statement).
> >> Doing this can't easily be made to work for the
> >> do_futimes() case without reworking the arguments passed to
> >> notify_change().  Actually, I'm inclined to doubt whether it
> >> is a good idea to try to roll that check into notify_change() --
> >> at least for utimensat() it seems simpler to not do so.
> >
> > Ugh...  Could we just omit this part (the if !times and write error
> > then check owner)?  I know it was my idea, but
> >
> >  a) my ideas are often stupid
> >  b) one patch should ideally do just one thing
> >
> > After we fixed the original issue, we can still think about this other
> > thing :)
> 
> Okay, by now quite a bit of my time has been wasted, and my patience
> is starting to get a little thin.

I understand your frustration, but actually I did say this the last
time as well:

  "Sorry, that was just an idea, but since it's not as simple as it
   should be, I guess we should leave that till later.  My main
   objection was against introducing more is_owner_or_cap() checks.
   Just doing the times == NULL case with ATTR_OWNER_CHECK should be
   fine."

Yeah, I may have been more explicit...

> The relevant interfaces here are:
> 
> utimensat()
> futimesat()
> utime()
> utimes()
> futimens() -- because implemented in glibc via utimensat() with path==NULL.
> 
> * utime() and utimes() can't be affected by this point: they don't use
> file descriptors.

OK, you're right.  I've overlooked this point.

So as long as we believe that nobody is using the futime*() interfaces
in the way you described, which is highly probable, then we can fix
that as well.  Which is actually a nice thing, because it means the
permission checking for the times == NULL case can move from both
do_utimes_name() and do_futimes() into utimes_common().

So let's make two patches, and let's forget about the write_error
thing for now:

1)
  - turn UTIME_NOW/UTIME_NOW into times = NULL
  - for times != NULL set ATTR_OWNER_CHECK

2)
  - move times == NULL permission check into utimes_common.

For 2) you can just use permission() instead of vfs_permission() which
is exactly the same in this case (and consolidated into a common
function later in the vfs-cleanups tree).

OK?

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/