linux-kernel - Re: [PATCH] drm/msm: Fix MSM_INFO_GET

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a9479d08-e9db-dfa7-c2f5-a8de5a0a28c4@gmail.com>
Date:   Fri, 8 Jan 2021 15:56:36 +0200
From:   Iskren Chernev <iskren.chernev@...il.com>
To:     Rob Clark <robdclark@...il.com>
Cc:     ~postmarketos/upstreaming@...ts.sr.ht, Sean Paul <sean@...rly.run>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout



On 1/8/21 12:36 AM, Rob Clark wrote:
> On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@...il.com> wrote:
>>
>> On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@...il.com> wrote:
>>>
>>> The msm_gem_get_iova should be guarded with gpu != NULL and not aspace
>>> != NULL, because aspace is NULL when using vram carveout.
>>>
>>> Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances")
>>>
>>> Signed-off-by: Iskren Chernev <iskren.chernev@...il.com>
>>> ---
>>>  drivers/gpu/drm/msm/msm_drv.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index c5e61cb3356df..c1953fb079133 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
>>>                 struct drm_file *file, struct drm_gem_object *obj,
>>>                 uint64_t *iova)
>>>  {
>>> +       struct msm_drm_private *priv = dev->dev_private;
>>>         struct msm_file_private *ctx = file->driver_priv;
>>>
>>> -       if (!ctx->aspace)
>>> +       if (!priv->gpu)
>>>                 return -EINVAL;
>>
>> Does this actually work?  It seems like you would hit a null ptr deref
>> in msm_gem_init_vma().. and in general I think a lot of code paths
>> would be surprised by a null address space, so this seems like a risky
>> idea.
>
> oh, actually, I suppose it is ok, since in the vram carveout case we
> create the vma up front when the gem obj is created..
>
> (still, it does seem a bit fragile.. and easy for folks testing on
> devices not using vram carvout to break.. hmm..)

In _msm_gem_new add_vma is called with NULL, so consequently lookup_vma
finds it when aspace is NULL.

Also, this is how the code was before the "breaking" change, so it should
not be worse.

I'll be happy to work on refactoring this a bit, but some some
documentation about the different gpu/mdp pieces and how they fit together
won't hurt.

Regards,
Iskren

> BR,
> -R
>
>> Maybe instead we should be creating an address space for the vram carveout?
>>
>> BR,
>> -R
>>
>>
>>>         /*
>>> --
>>> 2.29.2
>>>