On Mon, Jul 2, 2018 at 1:41 PM Mathieu Desnoyers
<mathieu.desnoy...@efficios.com> wrote:
>
> - if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
> + if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)) ||
> + rseq_cs->abort_ip >= TASK_SIZE)
> return -EFAULT;
I think the abort_ip check should have the same error value as the
other sanity checks, ie just be of this format:
> if (rseq_cs->version > 0)
> return -EINVAL;
also, I think you should check start_ip to be consistent. You kind of
accidentally do it with the check for
if (rseq_cs->abort_ip - rseq_cs->start_ip - rseq_cs->post_commit_offset)
but honestly, that has underflow issues already, so I think you want
to basically make the check be
if (rseq_cs->abort_ip >= TASK_SIZE)
return -EINVAL;
if (rseq_cs->start_ip >= rseq_cs->abort_ip)
return -EINVAL;
which takes care of checkint start_ip, and also the underflow for the
post_commit_offset check.
If somebody is depending on negative offsets, then that
post_commit_offset logic is already wrong.
> + usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
> ret = get_user(sig, usig);
That can underflow too, but I guess we can just rely on get_user()
getting it right.
Linus
