On Thu, Feb 15, 2018 at 02:59:14PM -0500, Olga Kornievskaia wrote:
> On Thu, Jan 25, 2018 at 5:04 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> > Nit: this could use a better subject line.
> >
> > On Tue, Oct 24, 2017 at 01:47:47PM -0400, Olga Kornievskaia wrote:
> > ...
> >> + if (!copy->cp_synchronous) {
> >> + status = nfsd4_init_copy_res(copy, 0);
> >> + async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> + if (!async_copy) {
> >> + status = nfserrno(-ENOMEM);
> >> + goto out;
> >> + }
> >> + dup_copy_fields(copy, async_copy);
> >> + memcpy(©->cp_res.cb_stateid, ©->cp_dst_stateid,
> >> + sizeof(copy->cp_dst_stateid));
> >> + spin_lock(&async_copy->cp_clp->async_lock);
> >> + list_add(&async_copy->copies,
> >> + &async_copy->cp_clp->async_copies);
> >> + spin_unlock(&async_copy->cp_clp->async_lock);
> >
> > At this point other threads could in theory look up this async_copy, but
> > its copy_task field is not yet initialized. I don't *think* that's a
> > problem for nfsd4_shutdown_copy, because I don't think the server could
> > be processing rpc's for this client any more at that point. But I think
> > a malicious client might be able to trigger a NULL dereference in
> > nfsd4_offload_cancel.
> >
> > Is there any reason not to assign copy_task before adding it to this
> > list?
>
> Now that I'm making changes I don't believe this is an issue. A client
> can't send nfsd4_offload_cancel() because it needs a copy stateid to
> send it with. And at this point the copy has not been replied to.
Right, but a malicious client might guess that copy stateid before it
gets the reply.
We want to make sure we're safe from crashing even on input that is very
unlikely.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
