Date: Tue, 26 Sep 2017 15:35:23 -0400
From: Jan Harkes <jaharkes@cs.cmu.edu>
To: Meng Xu <mengxu.gatech@gmail.com>
Cc: linux-kernel@vger.kernel.org, Meng Xu <meng.xu@gatech.edu>,
        sanidhya@gatech.edu, Taesoo Kim <taesoo@gatech.edu>
Subject: Re: [PATCH] fs/coda: ensure the header peeked at is the same in the
 actual message
Message-ID: <20170926193522.aniynxrzgfied2jk@cs.cmu.edu>
Mail-Followup-To: Meng Xu <mengxu.gatech@gmail.com>,
        linux-kernel@vger.kernel.org, Meng Xu <meng.xu@gatech.edu>,
        sanidhya@gatech.edu, Taesoo Kim <taesoo@gatech.edu>
References: <1505834623-37096-1-git-send-email-mengxu.gatech@gmail.com>
 <2A262D40-191F-4C3E-9BF8-CCF2289BE547@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2A262D40-191F-4C3E-9BF8-CCF2289BE547@gmail.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2544
Lines: 52

On Sat, Sep 23, 2017 at 10:35:45PM -0400, Meng Xu wrote:
> Hi Jaharkes and Coda filesystem developers,
> 
> I am resending the email on a potential race condition bug I found in the
> Coda filesystem as well as the patch I propose. Please feel free to comment
> whether you think this is a serious problem and whether the patch will work.
> Thank you.

Hi,

It does look like there is a potential race there but I don't believe it
is very serious because,

- The userspace Coda cache manager component (Coda client) is using
  co-routines instead of true multithreading. So even if someone tries
  to exploit this through a vulnerability through the Coda client the
  whole process is blocked on the write systemcall and there is no other
  thread of execution available that can try to change the fields.

- If someone can exploit the Coda client, they already have achieved
  root, so no need to try to exploit anything in the kernel.

- If someone can replace the opcode and unique id, they can replace
  anything else in the message, with much worse results. For instance by
  rewriting the response to an open call from success to failure the
  attacker is able to cause a file descriptor leak because the Coda
  client believes there is an open file, while the kernel and
  application believe the open failed, etc.

Now there are two types of messages the Coda client writes to the kernel
device.

- Downcalls which provide hints to trigger cache revalidations, here the
  unique id is ignored and the opcode mostly indicates how severe the
  cache invalidation is (only a single object, whole subtree, etc.) If
  someone wants to mess with that rewriting the file identifier that is
  in the message body is more useful and the worst they can do is make
  stale data stick in the kernel cache. I notice that your patch does
  not address the copy_from_user for downcalls around line 128.

- The other type are the upcall responses, file system requests are
  blocked until the Coda client returns a response with the matching
  unique id from the request. This match is performed at line 158, which
  is after the peek, but before the second copy_from_user. Once the
  matching request has been found and dequeued we do not look at the
  unique id anymore. Because the response is correlated to a specific
  request and we do not re-copy the opcode from the response to the
  req->uc_opcode field it doesn't really matter what the opcode in the
  response was at this point because all decisions are based on the
  request opcode.

Jan