From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D60D9C48BC2 for ; Fri, 25 Jun 2021 13:51:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C413360241 for ; Fri, 25 Jun 2021 13:51:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231683AbhFYNyJ (ORCPT ); Fri, 25 Jun 2021 09:54:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48984 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231627AbhFYNyH (ORCPT ); Fri, 25 Jun 2021 09:54:07 -0400 Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 382FCC061766 for ; Fri, 25 Jun 2021 06:51:46 -0700 (PDT) Received: by mail-qt1-x831.google.com with SMTP id g3so3226128qth.11 for ; Fri, 25 Jun 2021 06:51:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=GbSbNNqFKiu9Mctgx4Wsj99Xj0cOSJ9rf4uWMxWFhVY=; b=d/E1my5NgFegXWc2dnxbA8ZlWYIYgsn0cmMwOo+xHc07acGEwbKUi0EyZn3gld9PYh 4YJvV7MjBv+PXeBsn3yTvZoShM8LqsEuAd/iUDNYFgRF43ei5iiyv3RtEA1MCS+c5qBM LnhqE06g6w0uQnhHQO4jrYPBHluD2MEah70tPTH7QwoH9rCtj24r6RYo3BjSA3FQAozf H6LUxpfPBtC0QroTZx7/p4x/DtlZz7qGSE+4uuZECZqlxq1tMJRZ/s9Ro65b7v1UE+wf NmxaPh2j/bSq0lO90lXu2OQW6guR3iJkMyo8K0VLGhCyIqBnYWgakujEDC85IDmCYSBJ G5+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=GbSbNNqFKiu9Mctgx4Wsj99Xj0cOSJ9rf4uWMxWFhVY=; b=JhKDng6N53eQ0lctZB7ARnLTgu6aYiheqKxRAJ4POrwUR/CFaW4IRbARlAvTMNErJG +m32WrXRbxI0bIG9ThpBdeZ5M6qqg1oVqmb91jpXMVcOrlTn6+vA9wuyoCINSPTvbSFm ZL2+T0RTNWFc/MKjX1edGUOiKg5yhdExa1ix9nwxdfg6cO4DAVfloGtzpFFTa3pQTGzu hdBuo2KmFc8T71AN9AfXCenU1qwVkjLgEgppUZNZBTKxEejp+QuJquOLWBPHHeZW+5Y/ lr1Fgurp4SumJHdGOOHmBCKJOnCEqTuGAmiFZgOHt58ztPIJbRrIcLNMbS1o8eNEikv/ ckIw== X-Gm-Message-State: AOAM532YSbhwTN4ZSIFrfZzBGSYoR49RZpr2HSIZmmAj4k//HBDdVOQU qWegRFslba9BiVfL4ZvFPxZpdgCS779636zpKviojw== X-Google-Smtp-Source: ABdhPJzWIBGCrYgEsbZ2B7WYA3Nw2NgDvJvNE6RynWDcDD9/I3Lwrg2cwUNGChWnr9VjFdeLmbuwhBkOKWvUm6whJ3E= X-Received: by 2002:a05:622a:1011:: with SMTP id d17mr9795664qte.66.1624629105091; Fri, 25 Jun 2021 06:51:45 -0700 (PDT) MIME-Version: 1.0 References: <000000000000e61e2105c58fea48@google.com> <20210625085140.1735-1-hdanton@sina.com> <20210625094638.1791-1-hdanton@sina.com> <20210625130813.84-1-hdanton@sina.com> In-Reply-To: <20210625130813.84-1-hdanton@sina.com> From: Dmitry Vyukov Date: Fri, 25 Jun 2021 15:51:32 +0200 Message-ID: Subject: Re: [syzbot] KASAN: use-after-free Read in v4l2_ioctl (2) To: Hillf Danton Cc: syzbot , ezequiel@collabora.com, hverkuil-cisco@xs4all.nl, lijian@yulong.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, mchehab@kernel.org, sakari.ailus@linux.intel.com, syzkaller-bugs@googlegroups.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 25, 2021 at 3:08 PM Hillf Danton wrote: > >> >> Given the uaf in the ioctl path, open count is needed and should be > >> >> maintained by stk and is implemented in the diff below with mutex - it > >> >> is locked at file open time, released at file release time and aquired > >> >> at disconnect time. > >> >> > >> >> This can be a quick fix to the uaf, though, lights on why the video_get(vdev) > >> >> in v4l2_open() fails to prevent stk camera from going home too early are > >> >> welcome. Is it the fault on the driver side without an eye on open count? > >> >> > >> >> +++ x/drivers/media/usb/stkwebcam/stk-webcam.c > >> >> @@ -624,8 +624,10 @@ static int v4l_stk_open(struct file *fp) > >> >> dev->first_init = 0; > >> >> > >> >> err = v4l2_fh_open(fp); > >> >> - if (!err) > >> >> + if (!err) { > >> >> usb_autopm_get_interface(dev->interface); > >> >> + mutex_trylock(&dev->free_mutex); > >> > > >> >I haven't read all of it, but doing mutex_trylock w/o checking the > >> >return value looks very fishy. Can it ever be the right thing to > >> >do?... E.g. the next line we unconditionally do mutex_unlock, are we > >> >potentially unlocking a non-locked mutex? > >> > >> I am having difficulty understanding your point until I see next line... > > > >Right, the next line unlocks a different mutex, so ignore the part > >about the next line. > > > >But I am still confused about the intention of trylock w/o using the > >return value. I fail to imagine any scenarios where it's the right > >thing to do. > > Let me explain. The whole point of the added mutex is solely to prevent > the disconnector from freeing the stk camera while there are still > openers of the video device, and trylock is used to walk around deadlock > because multiple openers are allowed. In function it is equivelant to the > usual method on top of open count and waitqueue, something like > > mutex_lock; > stk_cam->open_cnt++; // mutex_trylock(&stk_cam->free_mutex); > mutex_unlock; > > at file open time, and > > mutex_lock; > stk_cam->open_cnt = 0; > wake_up(&stk_cam->waitq); // mutex_unlock(&stk_cam->free_mutex); > mutex_unlock; > > at file release time, and > > wait_event(stk_cam->waitq, > stk_cam->open_cnt == 0); // mutex_lock(&stk_cam->free_mutex); > // mutex_unlock(&stk_cam->free_mutex); > kfree(stk_cam); > > at disconnect time, but has fewer lines of code to type. But if trylock has failed, then the file release will still unlock the mutex and unlocking a mutex without a prior lock is not permitted. Or, I assume disconnect needs to wait for all files to be released. This won't be the case with a mutex, because when the first file is released, mutex is unlocked and disconnect can proceed. But maybe I am still missing something. Are you sure your use of mutex complies with the rules? https://elixir.bootlin.com/linux/v5.13-rc7/source/include/linux/mutex.h#L31 > What is more crucial however is why the mechanisms in video core are > failing to prevent uaf like this one from coming true. Lets wait for > lights from the video folks.