linux-kernel - Re: old->umask copying without spin_lock, in copy_fs

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] [day] [month] [year] [list]

Message-ID: <CADZ9YHiROer+sNAVz5TVAdFB3ec6tiJ96aKAMh6YUn=Dom5e2w@mail.gmail.com>
Date:	Sun, 7 Apr 2013 14:49:54 +0600
From:	Rakib Mullick <rakib.mullick@...il.com>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: Re: old->umask copying without spin_lock, in copy_fs_struct()

On Sun, Apr 7, 2013 at 1:56 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Sun, Apr 07, 2013 at 11:37:27AM +0600, Rakib Mullick wrote:
>> Hello,
>>
>> In copy_fs_struct(), old->umask is assigned to fs->umask outside of
>> spin_lock(&old->lock). Shouldn't it be inside spin_lock()? Since we're
>> dealing with  fs_struct *old ? Isn't it unsafe? Following lines -
>>
>>               fs->umask = old->umask;
>>
>>                 spin_lock(&old->lock);
>
> What would moving it down buy us?  Root, pwd and umask are all modified
> independently; the *only* reason why we hold old->lock for root and
> pwd (and we might drop and regain it between copying those - it would
> be pointless, so we don't bother, but it wouldn't have affected correctness)
> is that we want the values of root.mnt and root.dentry taken at the same
> time and we want to grab extra references on those while they are still
> valid.  The same goes for pwd, of course.  That's what old->lock
> protects - we want the damn thing atomic wrt set_fs_root() and set_fs_pwd().
> umask is an integer; its updates are atomic anyway, so it's not as if we
> could see a half-updated value or needed to do anything with refcounts.

Thanks for your explanation! The ->umask operation is trivial and as
you've explained (I was also looking at the code),
it seems that code execution order makes sure that nothing goes wrong.
fs_struct's data are protected with the ->lock, that's what I was
thinking in that way and was just making sure it wasn't missed out
accidentally.

Thanks
Rakib.
--
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/