Re: Q: proc: disable mem_write after exec

[Stephen Wilson]

On Thu, Sep 29, 2011 at 04:17:06PM +0200, Oleg Nesterov wrote:
> The last question, I promise ;)

No worries :)

> 	@@ -850,6 +850,10 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
> 		if (check_mem_permission(task))
> 			goto out;
> 	 
> 	+	copied = -EIO;
> 	+	if (file->private_data != (void *)((long)current->self_exec_id))
> 	+		goto out;
> 	+
> 		copied = -ENOMEM;
> 		page = (char *)__get_free_page(GFP_TEMPORARY);
> 		if (!page)
> 
> 
> Could you explain this? Why this is wrong from the security pov? We are
> the tracer, this was checked by check_mem_permission(). We can modify
> this memory even without /proc/pid/mem.

As far as I understand this is really just a paranoia thing, not a
security issue in any deep sense. If the fd is leaked across an exec()
then the target process memory could be written to by accident. So the
intent here was to protect against buggy userspace more than anything
else. 


> And in any case, why do we check current's self_exec_id? I'd understand
> if mem_open/mem_read/mem_writed used task->self_exec_id, see the trivial
> patch below. With this patch this check means: this task has changed its
> ->mm after /proc/pid/mem was opened, abort. And perhaps this was the
> actual intent. May be makes sense.

Perhaps, but my feeling is that the tracer should be prepared to deal
with that.  From the user perspective I think of /proc/pid/mem as being
associated with a pid, not an mm, and I can't see the benefit of going
thru a close()/open() cycle to deal with an event that can be detected
using ptrace.


> But the real question is, why (from the security pov) we can't simply kill
> these self_exec_id checks?
> 
> Not to mention, it would be nice to remove self_exec_id/parent_exec_id too.
> Ignoring mem_open(), afaics the _only_ reason we need these id's is that
> linux allows clone(CLONE_PARENT | SIGKILL).

So I think this is just a case of weighing the costs and benefits.
Personally, I think having /proc/pid/mem implicitly CLOEXEC is the
"right" thing to do.  But I wouldn't argue if the checks were dropped in
an effort to simplify code.



Thanks,


-- 
steve

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