| 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
| ||
|
Message-ID: <7ejauhf33nyt77ve26524rtvf46qohipztydjnbezlaz666cpg@3wfhpaxv3rd2>
Date: Thu, 19 Sep 2024 11:46:16 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>
Cc: srinivas.kandagatla@...aro.org, linux-arm-msm@...r.kernel.org,
gregkh@...uxfoundation.org, quic_bkumar@...cinc.com, linux-kernel@...r.kernel.org,
quic_chennak@...cinc.com, dri-devel@...ts.freedesktop.org, arnd@...db.de,
stable <stable@...nel.org>
Subject: Re: [PATCH v1 3/3] misc: fastrpc: Skip reference for DMA handles
On Thu, Sep 19, 2024 at 12:00:33PM GMT, Ekansh Gupta wrote:
>
>
> On 8/30/2024 3:03 PM, Dmitry Baryshkov wrote:
> > On Thu, Aug 22, 2024 at 04:29:33PM GMT, Ekansh Gupta wrote:
> >> If multiple dma handles are passed with same fd over a remote call
> >> the kernel driver takes a reference and expects that put for the
> >> map will be called as many times to free the map.
> >> But DSP only
> >> updates the fd one time in the fd list when the DSP refcount
> >> goes to zero
> > I'm sorry, I couldn't understand this phrase. Could you plese clarify
> > what do you mean here?
> DMA handle are buffers passed to DSP which are only unmapped when DSP updated
> the buffer fd in fdlist.
> fdlist implementation: misc: fastrpc: Add fdlist implementation - kernel/git/next/linux-next.git - The linux-next integration testing tree <https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/misc/fastrpc.c?id=8f6c1d8c4f0cc316b0456788fff8373554d1d99d>
>
> A remote call payload carries both input/output buffers and dma handles. The lifetime
> of input/output buffer is a remote call which means that any buffer allocated or mapped
> for a remote call will be freed or unmapped when the remote call is completing. Whereas,
> dma handles can get freed over some other remote call whenever the DSP will update
> fdlist. So if a remote call passed multiple dma handles with same fd to DSP, on driver, ref
> count will be incremented, but DSP can update fdlist only 1 time for the same fd as DSP also
> has a ref counting happening for the dma handle and fdlist is updated when the DSP ref
> count goes to 0. In this case, the map will not get freed even though it is no longer in use.
OK, I started looking at the related code. Pleas fix possible map leak
in fastrpc_put_args(), happening if the copy_to_user() fails.
Second. Please merge fastrpc_map_lookup() + fastrpc_map_put() invocation
into a single call, effectively dropping take_ref argument from
fastrpc_map_lookup() (which can now become fastrpc_map_get()).
Now back to your patch.
Please clarify if my understanding is correct:
The driver maps dma bufs and passes them to DSP. Then once DSP firmware
finds out that a particular buffer is no longer needed, it returns
its fd via the fdlist part of the invoke_buf. As these buffers are
returned only once, when they are no longer necessary, the kernel should
not take additional references on the long-living dma-bufs.
If that's the case, see my comments below.
> >
> >> and hence kernel make put call only once for the
> >> fd. This can cause SMMU fault issue as the same fd can be used
> >> in future for some other call.
> >>
> >> Fixes: 35a82b87135d ("misc: fastrpc: Add dma handle implementation")
> >> Cc: stable <stable@...nel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
> >> ---
> >> drivers/misc/fastrpc.c | 13 ++++++++-----
> >> 1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index ebe828770a8d..ad56e918e1f8 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -755,7 +755,7 @@ static const struct dma_buf_ops fastrpc_dma_buf_ops = {
> >>
> >> static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> >> u64 va, u64 len, u32 attr,
> >> - struct fastrpc_map **ppmap)
> >> + struct fastrpc_map **ppmap, bool take_ref)
> >> {
> >> struct fastrpc_session_ctx *sess = fl->sctx;
> >> struct fastrpc_map *map = NULL;
> >> @@ -763,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> >> struct scatterlist *sgl = NULL;
> >> int err = 0, sgl_index = 0;
> >>
> >> - if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, true))
> >> + if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, take_ref))
> >> return 0;
Do not add the take_ref argument to fastrpc_map_create(). Instead
extract the rest of the code to the function fastrpc_map_attach() (or
something like that).
> >>
> >> map = kzalloc(sizeof(*map), GFP_KERNEL);
> >> @@ -917,14 +917,17 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx)
> >> int i, err;
> >>
> >> for (i = 0; i < ctx->nscalars; ++i) {
> >> + bool take_ref = true;
> >>
> >> if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1 ||
> >> ctx->args[i].length == 0)
> >> continue;
> >>
> >> + if (i >= ctx->nbufs)
> >> + take_ref = false;
> > Please clarify too.
> nbufs -> total input/output buffers
> nscalars -> nbufs + dma handles
> So here, avoiding ref increment for dma handles.
> >
> >> err = fastrpc_map_create(ctx->fl, ctx->args[i].fd,
> >> (u64)ctx->args[i].ptr, ctx->args[i].length,
> >> - ctx->args[i].attr, &ctx->maps[i]);
> >> + ctx->args[i].attr, &ctx->maps[i], take_ref);
Call conditionally either fastrpc_map_create() or fastrpc_map_attach().
> >> if (err) {
> >> dev_err(dev, "Error Creating map %d\n", err);
> >> return -EINVAL;
> >> @@ -1417,7 +1420,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >>
> >> if (init.filelen && init.filefd) {
> >> err = fastrpc_map_create(fl, init.filefd, init.file,
> >> - init.filelen, 0, &map);
> >> + init.filelen, 0, &map, true);
> >> if (err)
> >> goto err;
> >> }
> >> @@ -2040,7 +2043,7 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
> >>
> >> /* create SMMU mapping */
> >> err = fastrpc_map_create(fl, req.fd, req.vaddrin, req.length,
> >> - 0, &map);
> >> + 0, &map, true);
> >> if (err) {
> >> dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
> >> return err;
> >> --
> >> 2.34.1
> >>
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists