linux-ext4 - Re: [PATCH 01/17] mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171024130808.GC8556@quack2.suse.cz>
Date:   Tue, 24 Oct 2017 15:08:08 +0200
From:   Jan Kara <jack@...e.cz>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>, linux-nvdimm@...ts.01.org,
        linux-api@...r.kernel.org, linux-xfs@...r.kernel.org,
        Andy Lutomirski <luto@...nel.org>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 01/17] mm: introduce MAP_SHARED_VALIDATE, a mechanism to
 safely define new mmap flags

On Fri 20-10-17 00:27:07, Christoph Hellwig wrote:
> >  	if (file) {
> >  		struct inode *inode = file_inode(file);
> > +		unsigned long flags_mask = file->f_op->mmap_supported_flags;
> > +
> > +		if (!flags_mask)
> > +			flags_mask = LEGACY_MAP_MASK;
> >  
> >  		switch (flags & MAP_TYPE) {
> >  		case MAP_SHARED:
> > +			/*
> > +			 * Silently ignore unsupported flags - MAP_SHARED has
> > +			 * traditionally behaved like that and we don't want
> > +			 * to break compatibility.
> > +			 */
> > +			flags &= flags_mask;
> > +			/*
> > +			 * Force use of MAP_SHARED_VALIDATE with non-legacy
> > +			 * flags. E.g. MAP_SYNC is dangerous to use with
> > +			 * MAP_SHARED as you don't know which consistency model
> > +			 * you will get.
> > +			 */
> > +			flags &= LEGACY_MAP_MASK;
> > +			/* fall through */
> > +		case MAP_SHARED_VALIDATE:
> > +			if (flags & ~flags_mask)
> > +				return -EOPNOTSUPP;
> 
> Hmmm.  I'd expect this to worth more like:
> 
> 		case MAP_SHARED:
> 			/* Ignore all new flags that need validation: */
> 			flags &= LEGACY_MAP_MASK;
> 			/*FALLTHROUGH*/
> 		case MAP_SHARED_VALIDATE:
> 			if (flags & ~file->f_op->mmap_supported_flags)
> 				return -EOPNOTSUPP;
> 
> with the legacy mask always implicitly support as indicated in my
> comment to the XFS patch.

I was thinking about this. Originally I thought that mmap_supported_flags
would allow also to declare some legacy flags as unsupported and also it
seemed as a nicer symmetric interface to me. But I guess the need to mask
out legacy flags is mostly theoretical so I'm fine giving that up. So I'll
change this as you suggest.

> Although even the ignoring in MAP_SHARED seems dangerous, but I guess
> we need that to keep strict backwards compatibility.  In world I'd
> rather do
> 
> 	case MAP_SHARED:
> 		if (flags & ~LEGACY_MAP_MASK)
> 			return -EINVAL;

Yes, I think just ignoring new flags for MAP_SHARED is safer...

								Honza

-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR