From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756486AbdJPSMM (ORCPT ); Mon, 16 Oct 2017 14:12:12 -0400 Received: from mail-it0-f52.google.com ([209.85.214.52]:51170 "EHLO mail-it0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753126AbdJPSMK (ORCPT ); Mon, 16 Oct 2017 14:12:10 -0400 X-Google-Smtp-Source: ABhQp+Srfp3wngH3wdfm/6wbXoKGrpCG1BCbfUncdVUEq5eWUl2DDMA6zBT3EWYNjJDmedjD5zFw7g== Date: Mon, 16 Oct 2017 11:12:06 -0700 From: Eric Biggers To: Ben Hutchings Cc: David Howells , stable@vger.kernel.org, Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 4.4 11/41] KEYS: fix writing past end of user-supplied buffer in keyring_read() Message-ID: <20171016181206.GC121701@google.com> References: <20171003114219.900672076@linuxfoundation.org> <20171003114220.627158480@linuxfoundation.org> <1508168854.22379.25.camel@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1508168854.22379.25.camel@codethink.co.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 16, 2017 at 04:47:34PM +0100, Ben Hutchings wrote: > On Tue, 2017-10-03 at 14:21 +0200, Greg Kroah-Hartman wrote: > > 4.4-stable review patch.  If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Eric Biggers > > > > commit e645016abc803dafc75e4b8f6e4118f088900ffb upstream. > > > > Userspace can call keyctl_read() on a keyring to get the list of IDs of > > keys in the keyring.  But if the user-supplied buffer is too small, the > > kernel would write the full list anyway --- which will corrupt whatever > > userspace memory happened to be past the end of the buffer.  Fix it by > > only filling the space that is available. > [...] > > trusted_read() has the same bug. > > Also, the comment above keyctl_read_key() says "return the amount of > data that is available in the key, irrespective of how much we copied > into the buffer." All the other implementations of key_type::read seem > to follow that, but this changes keyring_read() to return buflen in > case of a truncated read. > Hi Ben, thanks for pointing this out. I had assumed the "obvious" semantics, but it turns out that's not what's documented. And actually the comment above keyctl_read_key() might be wrong too, since the man page says that nothing is copied if the buffer is too small... David, which behavior was intended? Some of the ->read() methods will fill a too-small buffer, while others don't. I think this bug isn't *too* bad since the short return will only be encountered in cases where the kernel would have corrupted userspace memory before. But I'll send a patch to fix it. And yes, trusted_read() is ignoring 'buflen' which is a bug too; I'll see if I can fix that too... Eric From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f45.google.com ([209.85.214.45]:47629 "EHLO mail-it0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754739AbdJPSMK (ORCPT ); Mon, 16 Oct 2017 14:12:10 -0400 Received: by mail-it0-f45.google.com with SMTP id p138so2338050itp.2 for ; Mon, 16 Oct 2017 11:12:10 -0700 (PDT) Date: Mon, 16 Oct 2017 11:12:06 -0700 From: Eric Biggers To: Ben Hutchings Cc: David Howells , stable@vger.kernel.org, Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 4.4 11/41] KEYS: fix writing past end of user-supplied buffer in keyring_read() Message-ID: <20171016181206.GC121701@google.com> References: <20171003114219.900672076@linuxfoundation.org> <20171003114220.627158480@linuxfoundation.org> <1508168854.22379.25.camel@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1508168854.22379.25.camel@codethink.co.uk> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Oct 16, 2017 at 04:47:34PM +0100, Ben Hutchings wrote: > On Tue, 2017-10-03 at 14:21 +0200, Greg Kroah-Hartman wrote: > > 4.4-stable review patch.��If anyone has any objections, please let me know. > > > > ------------------ > > > > From: Eric Biggers > > > > commit e645016abc803dafc75e4b8f6e4118f088900ffb upstream. > > > > Userspace can call keyctl_read() on a keyring to get the list of IDs of > > keys in the keyring.��But if the user-supplied buffer is too small, the > > kernel would write the full list anyway --- which will corrupt whatever > > userspace memory happened to be past the end of the buffer.��Fix it by > > only filling the space that is available. > [...] > > trusted_read() has the same bug. > > Also, the comment above keyctl_read_key() says "return the amount of > data that is available in the key, irrespective of how much we copied > into the buffer." All the other implementations of key_type::read seem > to follow that, but this changes keyring_read() to return buflen in > case of a truncated read. > Hi Ben, thanks for pointing this out. I had assumed the "obvious" semantics, but it turns out that's not what's documented. And actually the comment above keyctl_read_key() might be wrong too, since the man page says that nothing is copied if the buffer is too small... David, which behavior was intended? Some of the ->read() methods will fill a too-small buffer, while others don't. I think this bug isn't *too* bad since the short return will only be encountered in cases where the kernel would have corrupted userspace memory before. But I'll send a patch to fix it. And yes, trusted_read() is ignoring 'buflen' which is a bug too; I'll see if I can fix that too... Eric