linux-kernel - Re: GPF in shm

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: <20151012185533.GD3170@linux-uzut.site>
Date:	Mon, 12 Oct 2015 11:55:33 -0700
From:	Davidlohr Bueso <dave@...olabs.net>
To:	"Kirill A. Shutemov" <kirill@...temov.name>
Cc:	Dmitry Vyukov <dvyukov@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	dave.hansen@...ux.intel.com, Hugh Dickins <hughd@...gle.com>,
	Joe Perches <joe@...ches.com>, sds@...ho.nsa.gov,
	Oleg Nesterov <oleg@...hat.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>, mhocko@...e.cz,
	gang.chen.5i5j@...il.com, Peter Feiner <pfeiner@...gle.com>,
	aarcange@...hat.com, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>, syzkaller@...glegroups.com,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: GPF in shm_lock ipc

On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:

>On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
>> diff --git a/ipc/shm.c b/ipc/shm.c
>> index 4178727..9615f19 100644
>> --- a/ipc/shm.c
>> +++ b/ipc/shm.c
>> @@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
>>  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>>  {
>> -	struct shm_file_data *sfd = shm_file_data(file);
>> +	struct file *vma_file = vma->vm_file;
>> +	struct shm_file_data *sfd = shm_file_data(vma_file);
>> +	struct ipc_ids *ids = &shm_ids(sfd->ns);
>> +	struct kern_ipc_perm *shp;
>>  	int ret;
>> +	rcu_read_lock();
>> +	shp = ipc_obtain_object_check(ids, sfd->id);
>> +	if (IS_ERR(shp)) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	if (!ipc_valid_object(shp)) {
>> +		ret = -EIDRM;
>> +		goto err;
>> +	}
>> +	rcu_read_unlock();
>> +
>
>Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?

Nothing, but that is later caught by shm_open() doing similar checks. We
basically end up doing a check between ->mmap() calls, which is fair imho.
Note that this can occur anywhere in ipc as IPC_RMID is a user request/cmd,
and we try to respect it -- thus you can argue this race anywhere, which is
why we have EIDRM/EINVL. Ultimately the user should not be doing such hacks
_anyway_. So I'm not really concerned about it.

Another similar alternative would be perhaps to make shm_lock() return an
error, and thus propagate that error to mmap return. That way we would have
a silent way out of the warning scenario (afterward we cannot race as we
hold the ipc object lock). However, the users would now have to take this
into account...

      [validity check lockless]
      ->mmap()
      [validity check lock]

>Shouldn't we bump shm_nattch here? Or some other refcount?

At least not shm_nattach, as that would acknowledge a new attachment after
a valid IPC_RMID. But the problem is also with how we check for marked for
deletion segments -- ipc_valid_object() checking the deleted flag. As such,
we always rely on explicitly checking against the deleted flag.
--
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/