On Sat, 2019-06-15 at 10:16 +0300, Leon Romanovsky wrote:
> On Fri, Jun 14, 2019 at 03:00:32PM -0400, Doug Ledford wrote:
> > On Thu, 2019-06-13 at 21:38 -0300, Jason Gunthorpe wrote:
> > > + if (ibdev)
> > > + ret = __ib_get_client_nl_info(ibdev, client_name,
> > > res);
> > > + else
> > > + ret = __ib_get_global_client_nl_info(client_name,
> > > res);
> > > +#ifdef CONFIG_MODULES
> > > + if (ret == -ENOENT) {
> > > + request_module("rdma-client-%s", client_name);
> > > + if (ibdev)
> > > + ret = __ib_get_client_nl_info(ibdev,
> > > client_name, res);
> > > + else
> > > + ret =
> > > __ib_get_global_client_nl_info(client_name, res);
> > > + }
> > > +#endif
> >
> > I was trying to put my finger on something yesterday while reading
> > the
> > code, and this change makes it more clear for me. Do we really
> > want to
> > limit the info type based on ibdev? It seems to me that all global
> > info retrieval should work whether you open a specific ibdev or
> > not.
> > It's only the things that need the ibdev to return the correct
> > response
> > that should require it. Right now we only have one global info
> > provider, but would it be better to do:
> >
> > if (!strcmp("rdma_cm", client_name))
> > ret = __ib_get_global_client_nl_info(client_name, res);
> > else
> > ret = __ib_get_client_nl_info(ibdev, client_name, res);
> >
> > The other thing I was wondering about was the module
> > loading. Every
> > attempt to load a module is a fork/exec cycle and a context switch
> > over
> > to modprobe and back, and we make no attempt here to keep each
> > invocation of the netlink query from requesting a module. I'm
> > concerned this is actually a potential DoS attack vector. I was
> > thinking we should track each client name that's valid, and only
> > try
> > each name once. I saw four module names: rdma_cm, umad, issm, and
> > uverbs. I'm wondering if we should have a static table in the
> > netlink
> > file with an entry for each of the client names and a variable to
> > indicate we've attempted to load that module, and on -ENOENT, we
> > check
> > the table for a match to our passed in client_name, and only if we
> > have
> > a match, and it's load count is 0, do we call request_module() and
> > increment the load count. Thoughts?
>
> Isn't request_module privileged operation, so only root or his
> friends
> can do this DDoS?
From what I can tell, SELinux *might* prevent a user from doing this,
but it is the only thing I could find that even tries to stop it.
Which of course means there are no protections for SELinux disabled
case. And I'm not sure SELinux stops it anyway. In general, autoload
isn't very useful if the only person that can trigger it is root. I
think in most cases, it is initiated by device file open or some such
that regular users can do. It's possible things are fine because the
core code serializes module autoload and 1,000,000 simultaneous
requests will result in one real request, and 999,999 waiters that wait
for the result of the 1 and then take that back. But as I read through
stuff, it's enough of a head scratcher that I'm tempted to try writing
an exploit and seeing what it does.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
