linux-kernel - re: drm/i915/selftests: Prepare gtt tests for obj->mm.lock removal

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>] [day] [month] [year] [list]
Message-ID: <5fac4ebb-e0aa-d628-1457-3feffea3b891@canonical.com>
Date:   Thu, 1 Apr 2021 14:59:19 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Cc:     Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        intel-gfx <intel-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: drm/i915/selftests: Prepare gtt tests for obj->mm.lock removal

Hi,

Static analysis with Coverity on Linux-next has detected a potential
issue with the following commit:

commit 480ae79537b28f30ef6e07b7de69a9ae2599daa7
Author: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Date:   Tue Mar 23 16:50:49 2021 +0100

    drm/i915/selftests: Prepare gtt tests for obj->mm.lock removal


The analysis by Coverity is as follows:

145 static int igt_ppgtt_alloc(void *arg)
146 {
147        struct drm_i915_private *dev_priv = arg;
148        struct i915_ppgtt *ppgtt;
   1. var_decl: Declaring variable ww without initializer.
149        struct i915_gem_ww_ctx ww;
150        u64 size, last, limit;
151        int err = 0;
152
153        /* Allocate a ppggt and try to fill the entire range */
154
   2. Condition !(dev_priv->__info.ppgtt_type != INTEL_PPGTT_NONE),
taking false branch.
155        if (!HAS_PPGTT(dev_priv))
156                return 0;
157
158        ppgtt = i915_ppgtt_create(&dev_priv->gt);
   3. Condition IS_ERR(ppgtt), taking false branch.
159        if (IS_ERR(ppgtt))
160                return PTR_ERR(ppgtt);
161
   4. Condition !ppgtt->vm.allocate_va_range, taking true branch.
162        if (!ppgtt->vm.allocate_va_range)
   5. Jumping to label err_ppgtt_cleanup.
163                goto err_ppgtt_cleanup;
164
165        /*
166         * While we only allocate the page tables here and so we could
167         * address a much larger GTT than we could actually fit into
168         * RAM, a practical limit is the amount of physical pages in
the system.
169         * This should ensure that we do not run into the oomkiller
during
170         * the test and take down the machine wilfully.
171         */
172        limit = totalram_pages() << PAGE_SHIFT;
173        limit = min(ppgtt->vm.total, limit);
174
175        i915_gem_ww_ctx_init(&ww, false);
176retry:
177        err = i915_vm_lock_objects(&ppgtt->vm, &ww);
178        if (err)
179                goto err_ppgtt_cleanup;
180
181        /* Check we can allocate the entire range */
182        for (size = 4096; size <= limit; size <<= 2) {
183                struct i915_vm_pt_stash stash = {};
184
185                err = i915_vm_alloc_pt_stash(&ppgtt->vm, &stash, size);
186                if (err)
187                        goto err_ppgtt_cleanup;
188
189                err = i915_vm_pin_pt_stash(&ppgtt->vm, &stash);
190                if (err) {
191                        i915_vm_free_pt_stash(&ppgtt->vm, &stash);
192                        goto err_ppgtt_cleanup;
193                }
194
195                ppgtt->vm.allocate_va_range(&ppgtt->vm, &stash, 0, size);
196                cond_resched();
197
198                ppgtt->vm.clear_range(&ppgtt->vm, 0, size);
199
200                i915_vm_free_pt_stash(&ppgtt->vm, &stash);
201        }
202
203        /* Check we can incrementally allocate the entire range */
204        for (last = 0, size = 4096; size <= limit; last = size, size
<<= 2) {
205                struct i915_vm_pt_stash stash = {};
206
207                err = i915_vm_alloc_pt_stash(&ppgtt->vm, &stash, size
- last);
208                if (err)
209                        goto err_ppgtt_cleanup;
210
211                err = i915_vm_pin_pt_stash(&ppgtt->vm, &stash);
212                if (err) {
213                        i915_vm_free_pt_stash(&ppgtt->vm, &stash);
214                        goto err_ppgtt_cleanup;
215                }
216
217                ppgtt->vm.allocate_va_range(&ppgtt->vm, &stash,
218                                            last, size - last);
219                cond_resched();
220
221                i915_vm_free_pt_stash(&ppgtt->vm, &stash);
222        }
223
224 err_ppgtt_cleanup:
   6. Condition err == -35, taking false branch.
225        if (err == -EDEADLK) {
226                err = i915_gem_ww_ctx_backoff(&ww);
227                if (!err)
228                        goto retry;
229        }
   7. uninit_use_in_call: Using uninitialized value ww.contended when
calling i915_gem_ww_ctx_fini.
   Uninitialized pointer read (UNINIT)
   8. uninit_use_in_call: Using uninitialized value ww.ctx.acquired when
calling i915_gem_ww_ctx_fini.
230        i915_gem_ww_ctx_fini(&ww);
231
232        i915_vm_put(&ppgtt->vm);
233        return err;
234 }

Coverity is reporting use of uninitialized values in (lines 230.  Not
sure what the best fix is for this, so I'm reporting this as a potential
issue.

Colin