Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Martin Lau <kafai@fb.com>
To:     Wenwen Wang <wang6495@umn.edu>
CC:     Kangjie Lu <kjlu@umn.edu>, Alexei Starovoitov <ast@kernel.org>,
        "Daniel Borkmann" <daniel@iogearbox.net>,
        "open list:BPF (Safe dynamic programs and tools)" 
        <netdev@vger.kernel.org>,
        "open list:BPF (Safe dynamic programs and tools)" 
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] bpf: btf: Fix a missing-check bug
Thread-Topic: [PATCH v2] bpf: btf: Fix a missing-check bug
Thread-Index: AQHUa5mXie9xmsybbEORoB7Wt/lhG6UuMIAAgACFY4A=
Date:   Wed, 24 Oct 2018 18:22:46 +0000
Message-ID: <20181024182239.lz7uicceihzmxabh@kafai-mbp>
References: <1540386020-30680-1-git-send-email-wang6495@umn.edu>
 <20181024172514.l33dsaqdvs5yewvm@kafai-mbp>
In-Reply-To: <20181024172514.l33dsaqdvs5yewvm@kafai-mbp>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2620:10d:c090:180::1:be1]
x-ms-publictraffictype: Email
x-microsoft-exchange-diagnostics: 1;MWHPR15MB1357;20:fiJVaFn8qTtEbMlISkts2u0ZFdV0bgAArjIx4iLv0YzYZHZ9VIu39pDsobCguq4V9Rzt8lf7nis2K3AfCe0B5mYJlkPpWYu0XBZhC/M6mgd1+ycOPhHdSIOFtOsvMBAsMZalWAlPB0O2zJlknNfpPzEemHVJVREHi3OR1WwZfxA=
x-ms-office365-filtering-correlation-id: 10208209-17ea-47b7-36fc-08d639ddb1e7
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020);SRVR:MWHPR15MB1357;
x-ms-traffictypediagnostic: MWHPR15MB1357:
x-microsoft-antispam-prvs: <MWHPR15MB135751AB3F8023C52E2A2929D5F60@MWHPR15MB1357.namprd15.prod.outlook.com>
x-exchange-antispam-report-test: UriScan:(192374486261705)(67672495146484)(8104003914727);
x-ms-exchange-senderadcheck: 1
x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231355)(11241501184)(944501410)(52105095)(3002001)(10201501046)(93006095)(93001095)(148016)(149066)(150057)(6041310)(20161123564045)(20161123562045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051)(76991095);SRVR:MWHPR15MB1357;BCL:0;PCL:0;RULEID:;SRVR:MWHPR15MB1357;
x-forefront-prvs: 083526BF8A
x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(7916004)(39860400002)(376002)(366004)(346002)(136003)(396003)(189003)(199004)(6116002)(8676002)(81166006)(81156014)(5250100002)(97736004)(478600001)(1076002)(8936002)(14454004)(476003)(46003)(33716001)(186003)(11346002)(446003)(7736002)(14444005)(256004)(305945005)(486006)(86362001)(68736007)(6246003)(229853002)(2171002)(4326008)(33896004)(76176011)(386003)(6506007)(6512007)(52116002)(25786009)(53936002)(6436002)(6486002)(2906002)(5660300001)(102836004)(6916009)(71200400001)(316002)(71190400001)(105586002)(2900100001)(106356001)(9686003)(54906003)(99286004);DIR:OUT;SFP:1102;SCL:1;SRVR:MWHPR15MB1357;H:MWHPR15MB1790.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1;
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
x-microsoft-antispam-message-info: b5JkKlBHdmxe0+1VsVNB6RUvE3JUQeAs+iTRMZfNCIw89fMAt/5c6N2pdiyKpSb8hX1HY4HcXbinW5jPmo4Ki88AYNRRAq7bQp8xlWaqsBh2fxLUdb7cLNsZf7H9sIJtRVeW0qtn13utlwUFzsM5VAQNzIl7ApeI44yGyteE4YRQDSiYs4nGcTtX0ayDDL5JEtImEyExeJX6Zqr4mzqyDoKWmf3Hz3bD7ml81zBfWEfONxzk+QTkqYCstgVPeUCwwVBrw9P9gEN3L2gZFh6yuZN/vwCQYzToUWmSFjJEXQBbAr/T7VBcF5okd7T66MD0cIRMpv3hvvYWeQAJDqAr3hfHSAMcR7vXQSbODgdolpY=
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <456989B783EEC440BE50DBA7A72D849F@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 10208209-17ea-47b7-36fc-08d639ddb1e7
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Oct 2018 18:22:46.6498
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1357
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-10-24_06:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> On Wed, Oct 24, 2018 at 08:00:19AM -0500, Wenwen Wang wrote:
> > In btf_parse(), the header of the user-space btf data 'btf_data' is fir=
stly
> > parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the he=
ader
> > is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and the=
n
> > verified. If no error happens during the verification process, the whol=
e
> > data of 'btf_data', including the header, is then copied to 'data' in
> > btf_parse(). It is obvious that the header is copied twice here. More
> > importantly, no check is enforced after the second copy to make sure th=
e
> > headers obtained in these two copies are same. Given that 'btf_data'
> > resides in the user space, a malicious user can race to modify the head=
er
> > between these two copies. By doing so, the user can inject inconsistent
> > data, which can cause undefined behavior of the kernel and introduce
> > potential security risk.
btw, I am working on a patch that copies the btf_data before parsing/verify=
ing
the header.  That should avoid this from happening but that will
require a bit more code churns for the bpf branch.

> >=20
> > To avoid the above issue, this patch copies the parsed header from
> > 'btf->hdr' to 'data'. The remaining part in 'data' is still copied from=
 the
> > user-space 'btf_data'.
> LGTM.
>=20
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>=20
> >=20
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> >  kernel/bpf/btf.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >=20
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 378cef7..b52a834a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_dat=
a, u32 btf_data_size,
> >  	struct btf_verifier_env *env =3D NULL;
> >  	struct bpf_verifier_log *log;
> >  	struct btf *btf =3D NULL;
> > +	u32 hdr_len;
> >  	u8 *data;
> >  	int err;
> > =20
> > @@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_da=
ta, u32 btf_data_size,
> >  	btf->data_size =3D btf_data_size;
> >  	btf->nohdr_data =3D btf->data + btf->hdr.hdr_len;
> > =20
> > -	if (copy_from_user(data, btf_data, btf_data_size)) {
> > +	/*
> > +	 * The header at btf_data could be modified by a malicious user
> > +	 * after it is parsed. So we copy the parsed header here. The
> > +	 * remaining part is still copied from btf_data.
> > +	 */
> > +	hdr_len =3D min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
> > +	memcpy(data, &btf->hdr, hdr_len);
> > +	if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
> > +				btf_data_size - hdr_len)) {
> >  		err =3D -EFAULT;
> >  		goto errout;
> >  	}
> > --=20
> > 2.7.4
> >=20