From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id A34F53858D33 for ; Tue, 7 Feb 2023 17:17:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A34F53858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 220063000A21; Tue, 7 Feb 2023 18:17:43 +0100 (CET) Received: by r6.localdomain (Postfix, from userid 1000) id D26DE3401EF; Tue, 7 Feb 2023 18:17:40 +0100 (CET) Message-ID: <7485818501031d589569f23e014b98df4e68cf74.camel@klomp.org> Subject: Re: [PATCH] libdw: check memory access in get_(u|s)leb128 From: Mark Wielaard To: Aleksei Vetrov Cc: elfutils-devel@sourceware.org, kernel-team@android.com, maennich@google.com Date: Tue, 07 Feb 2023 18:17:40 +0100 In-Reply-To: References: <20230125160530.949622-1-vvvvvv@google.com> <20230126210539.GC2781@gnu.wildebeest.org> Content-Type: multipart/mixed; boundary="=-DHAps9H/mwgXTVovTZ1g" User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) MIME-Version: 1.0 X-Spam-Status: No, score=-3036.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,KAM_NUMSUBJECT,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --=-DHAps9H/mwgXTVovTZ1g Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Aleksei, On Tue, 2023-02-07 at 16:17 +0000, Aleksei Vetrov wrote: > > Did you actually find situations where these functions were called > > with addrp > > > =3D endp? >=20 > Yes, for example libdw/libdw_form.c:91:7. >=20 Urgh. There are actually 3 places in that function that need a guard. Then I started reviewing all the library code and came up with more than 10 other places that had a guard missing (see attached). I'll clean this up and also audit elflint.c and readelf.c and submit a proper patch for that. > > It turns out that get_[su]leb128 dominates some operations and really d= oes > > have to be as fast as possible. So I do like to know what the impact is= of > > this change. >=20 > This patch just moves __libdw_max_len_uleb128 to the beginning of the fun= ction > and adds only one new if. So hopefully it shouldn't affect performance at= all. Yeah. At first I thought it might be unnecessary because we already have this check in the calling code. But given that I quickly found 10+ places were that wasn't true I don't feel very confident about that now... Then I thought maybe we can just remove the calling code checks and simply always check like you are proposing. But the checks are slightly different. The caller guards signal bad data errors, while yours are hardening checks that make sure no invalid data is read. It is better to have both. Even if it might slow down the code. I'll finish the audit of readelf.c and elflint.c and see if adding both those checks and your hardening checks don't pessimize the code too much. Hopefully it won't and we can just add all the extra checks. Cheers, Mark --=-DHAps9H/mwgXTVovTZ1g Content-Disposition: inline; filename="get-leb128-guard.patch" Content-Type: text/x-patch; name="get-leb128-guard.patch"; charset="UTF-8" Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL2xpYmR3L2NmaS5jIGIvbGliZHcvY2ZpLmMKaW5kZXggNmQwOGNhOTAuLmE3 MTc0NDA1IDEwMDY0NAotLS0gYS9saWJkdy9jZmkuYworKysgYi9saWJkdy9jZmkuYwpAQCAtMjM5 LDYgKzIzOSw3IEBAIGV4ZWN1dGVfY2ZpIChEd2FyZl9DRkkgKmNhY2hlLAogCiAJY2FzZSBEV19D RkFfb2Zmc2V0X2V4dGVuZGVkX3NmOgogCSAgZ2V0X3VsZWIxMjggKG9wZXJhbmQsIHByb2dyYW0s IGVuZCk7CisJICBjZmlfYXNzZXJ0IChwcm9ncmFtIDwgZW5kKTsKIAkgIGdldF9zbGViMTI4IChz Zl9vZmZzZXQsIHByb2dyYW0sIGVuZCk7CiAJb2Zmc2V0X2V4dGVuZGVkX3NmOgogCSAgb2Zmc2V0 ID0gc2Zfb2Zmc2V0ICogY2llLT5kYXRhX2FsaWdubWVudF9mYWN0b3I7CkBAIC0yOTQsNiArMjk1 LDcgQEAgZXhlY3V0ZV9jZmkgKER3YXJmX0NGSSAqY2FjaGUsCiAJICBnZXRfdWxlYjEyOCAocmVn bm8sIHByb2dyYW0sIGVuZCk7CiAJICAvKiBEV19GT1JNX2Jsb2NrIGlzIGEgVUxFQjEyOCBsZW5n dGggZm9sbG93ZWQgYnkgdGhhdCBtYW55IGJ5dGVzLiAgKi8KIAkgIG9mZnNldCA9IHByb2dyYW0g LSAoY29uc3QgdWludDhfdCAqKSBjYWNoZS0+ZGF0YS0+ZC5kX2J1ZjsKKwkgIGNmaV9hc3NlcnQg KHByb2dyYW0gPCBlbmQpOwogCSAgZ2V0X3VsZWIxMjggKG9wZXJhbmQsIHByb2dyYW0sIGVuZCk7 CiAJICBjZmlfYXNzZXJ0IChvcGVyYW5kIDw9IChEd2FyZl9Xb3JkKSAoZW5kIC0gcHJvZ3JhbSkp OwogCSAgcHJvZ3JhbSArPSBvcGVyYW5kOwpkaWZmIC0tZ2l0IGEvbGliZHcvZHdhcmZfY2hpbGQu YyBiL2xpYmR3L2R3YXJmX2NoaWxkLmMKaW5kZXggYzhjOGJiNjEuLjk2YTBkNjA4IDEwMDY0NAot LS0gYS9saWJkdy9kd2FyZl9jaGlsZC5jCisrKyBiL2xpYmR3L2R3YXJmX2NoaWxkLmMKQEAgLTcz LDEwICs3MywxMyBAQCBfX2xpYmR3X2ZpbmRfYXR0ciAoRHdhcmZfRGllICpkaWUsIHVuc2lnbmVk IGludCBzZWFyY2hfbmFtZSwKIAogICAgICAgaWYgKGF0dHJfZm9ybSA9PSBEV19GT1JNX2luZGly ZWN0KQogCXsKKwkgIGlmIChyZWFkcCA+PSBlbmRwKQorCSAgICBnb3RvIGludmFsaWQ7CiAJICBn ZXRfdWxlYjEyOCAoYXR0cl9mb3JtLCByZWFkcCwgZW5kcCk7CiAJICBpZiAoYXR0cl9mb3JtID09 IERXX0ZPUk1faW5kaXJlY3QgfHwKIAkgICAgICBhdHRyX2Zvcm0gPT0gRFdfRk9STV9pbXBsaWNp dF9jb25zdCkKIAkgICAgeworCSAgICBpbnZhbGlkOgogCSAgICAgIF9fbGliZHdfc2V0ZXJybm8g KERXQVJGX0VfSU5WQUxJRF9EV0FSRik7CiAJICAgICAgcmV0dXJuIE5VTEw7CiAJICAgIH0KZGlm ZiAtLWdpdCBhL2xpYmR3L2R3YXJmX2ZyYW1lX3JlZ2lzdGVyLmMgYi9saWJkdy9kd2FyZl9mcmFt ZV9yZWdpc3Rlci5jCmluZGV4IGJjZjNmYTAzLi5hNmI3YzRjMSAxMDA2NDQKLS0tIGEvbGliZHcv ZHdhcmZfZnJhbWVfcmVnaXN0ZXIuYworKysgYi9saWJkdy9kd2FyZl9mcmFtZV9yZWdpc3Rlci5j CkBAIC0xMDAsNiArMTAwLDExIEBAIGR3YXJmX2ZyYW1lX3JlZ2lzdGVyIChEd2FyZl9GcmFtZSAq ZnMsIGludCByZWdubywgRHdhcmZfT3Agb3BzX21lbVszXSwKIAljb25zdCB1aW50OF90ICpwID0g ZnMtPmNhY2hlLT5kYXRhLT5kLmRfYnVmICsgcmVnLT52YWx1ZTsKIAljb25zdCB1aW50OF90ICpl bmQgPSAoZnMtPmNhY2hlLT5kYXRhLT5kLmRfYnVmCiAJCQkgICAgICArIGZzLT5jYWNoZS0+ZGF0 YS0+ZC5kX3NpemUpOworCWlmIChwID49IGVuZCkKKwkgIHsKKwkgICAgX19saWJkd19zZXRlcnJu byAoRFdBUkZfRV9JTlZBTElEX0RXQVJGKTsKKwkgICAgcmV0dXJuIC0xOworCSAgfQogCWdldF91 bGViMTI4IChibG9jay5sZW5ndGgsIHAsIGVuZCk7CiAJYmxvY2suZGF0YSA9ICh2b2lkICopIHA7 CiAKZGlmZiAtLWdpdCBhL2xpYmR3L2R3YXJmX2dldGFiYnJldi5jIGIvbGliZHcvZHdhcmZfZ2V0 YWJicmV2LmMKaW5kZXggMTNiZWU0OTMuLjViMDIzMzNmIDEwMDY0NAotLS0gYS9saWJkdy9kd2Fy Zl9nZXRhYmJyZXYuYworKysgYi9saWJkdy9kd2FyZl9nZXRhYmJyZXYuYwpAQCAtNzcsNiArNzcs NyBAQCBfX2xpYmR3X2dldGFiYnJldiAoRHdhcmYgKmRiZywgc3RydWN0IER3YXJmX0NVICpjdSwg RHdhcmZfT2ZmIG9mZnNldCwKIAkJCSAgICAgICsgZGJnLT5zZWN0aW9uZGF0YVtJRFhfZGVidWdf YWJicmV2XS0+ZF9zaXplKTsKICAgY29uc3QgdW5zaWduZWQgY2hhciAqc3RhcnRfYWJicmV2cCA9 IGFiYnJldnA7CiAgIHVuc2lnbmVkIGludCBjb2RlOworICAvLyBXZSBzdGFydCBvZmYgd2l0aCBh YmJyZXZwIGF0IG9mZnNldCwgd2hpY2ggaXMgY2hlY2tlZCBhYm92ZS4KICAgZ2V0X3VsZWIxMjgg KGNvZGUsIGFiYnJldnAsIGVuZCk7CiAKICAgLyogQ2hlY2sgd2hldGhlciB0aGlzIGNvZGUgaXMg YWxyZWFkeSBpbiB0aGUgaGFzaCB0YWJsZS4gICovCmRpZmYgLS1naXQgYS9saWJkdy9kd2FyZl9n ZXRsb2NhdGlvbi5jIGIvbGliZHcvZHdhcmZfZ2V0bG9jYXRpb24uYwppbmRleCBkMGQ3ODE2My4u NGU4YzA0N2IgMTAwNjQ0Ci0tLSBhL2xpYmR3L2R3YXJmX2dldGxvY2F0aW9uLmMKKysrIGIvbGli ZHcvZHdhcmZfZ2V0bG9jYXRpb24uYwpAQCAtNTQ1LDcgKzU0NSw3IEBAIF9fbGliZHdfaW50ZXJu X2V4cHJlc3Npb24gKER3YXJmICpkYmcsIGJvb2wgb3RoZXJfYnl0ZV9vcmRlciwKIAljYXNlIERX X09QX2RlcmVmX3R5cGU6CiAJY2FzZSBEV19PUF9HTlVfZGVyZWZfdHlwZToKIAljYXNlIERXX09Q X3hkZXJlZl90eXBlOgotCSAgaWYgKHVubGlrZWx5IChkYXRhICsgMSA+PSBlbmRfZGF0YSkpCisJ ICBpZiAodW5saWtlbHkgKGRhdGEgKyAyID49IGVuZF9kYXRhKSkKIAkgICAgZ290byBpbnZhbGlk OwogCSAgbmV3bG9jLT5udW1iZXIgPSAqZGF0YSsrOwogCSAgZ2V0X3VsZWIxMjggKG5ld2xvYy0+ bnVtYmVyMiwgZGF0YSwgZW5kX2RhdGEpOwpkaWZmIC0tZ2l0IGEvbGliZHcvZHdhcmZfZ2V0c3Jj bGluZXMuYyBiL2xpYmR3L2R3YXJmX2dldHNyY2xpbmVzLmMKaW5kZXggMmMxZDdhNDAuLmRmMDAz YzVmIDEwMDY0NAotLS0gYS9saWJkdy9kd2FyZl9nZXRzcmNsaW5lcy5jCisrKyBiL2xpYmR3L2R3 YXJmX2dldHNyY2xpbmVzLmMKQEAgLTU3Miw2ICs1NzIsOCBAQCByZWFkX3NyY2xpbmVzIChEd2Fy ZiAqZGJnLAogCWdvdG8gaW52YWxpZF9kYXRhOwogCiAgICAgICBzaXplX3QgbmZpbGVzOworICAg ICAgaWYgKChzaXplX3QpIChsaW5lZW5kcCAtIGxpbmVwKSA8IDEpCisJZ290byBpbnZhbGlkX2Rh dGE7CiAgICAgICBnZXRfdWxlYjEyOCAobmZpbGVzLCBsaW5lcCwgbGluZWVuZHApOwogCiAgICAg ICBpZiAobmZvcm1zID09IDAgJiYgbmZpbGVzICE9IDApCmRpZmYgLS1naXQgYS9saWJkdy9lbmNv ZGVkLXZhbHVlLmggYi9saWJkdy9lbmNvZGVkLXZhbHVlLmgKaW5kZXggZDRlMDE5MjQuLjQ1NjZl Zjk2IDEwMDY0NAotLS0gYS9saWJkdy9lbmNvZGVkLXZhbHVlLmgKKysrIGIvbGliZHcvZW5jb2Rl ZC12YWx1ZS5oCkBAIC0xOTYsMTAgKzE5NiwxNCBAQCByZWFkX2VuY29kZWRfdmFsdWUgKGNvbnN0 IER3YXJmX0NGSSAqY2FjaGUsIHVpbnQ4X3QgZW5jb2RpbmcsCiAgICAgICBicmVhazsKIAogICAg IGNhc2UgRFdfRUhfUEVfdWxlYjEyODoKKyAgICAgIGlmICgqcCA+PSBlbmRwKQorCWdvdG8gaW52 YWxpZF9kYXRhOwogICAgICAgZ2V0X3VsZWIxMjggKHZhbHVlLCAqcCwgZW5kcCk7CiAgICAgICBi cmVhazsKIAogICAgIGNhc2UgRFdfRUhfUEVfc2xlYjEyODoKKyAgICAgIGlmICgqcCA+PSBlbmRw KQorCWdvdG8gaW52YWxpZF9kYXRhOwogICAgICAgZ2V0X3NsZWIxMjggKHZhbHVlLCAqcCwgZW5k cCk7CiAgICAgICBicmVhazsKIApkaWZmIC0tZ2l0IGEvbGliZHcvZmRlLmMgYi9saWJkdy9mZGUu YwppbmRleCBmNWY2ZmJlMS4uNzNkNTUxYjYgMTAwNjQ0Ci0tLSBhL2xpYmR3L2ZkZS5jCisrKyBi L2xpYmR3L2ZkZS5jCkBAIC0xMDQsOSArMTA0LDEyIEBAIGludGVybl9mZGUgKER3YXJmX0NGSSAq Y2FjaGUsIGNvbnN0IER3YXJmX0ZERSAqZW50cnkpCiAgICAgICAvKiBUaGUgQ0lFIGF1Z21lbnRh dGlvbiBzYXlzIHRoZSBGREUgaGFzIGEgRFdfRk9STV9ibG9jawogCSBiZWZvcmUgaXRzIGFjdHVh bCBpbnN0cnVjdGlvbiBzdHJlYW0uICAqLwogICAgICAgRHdhcmZfV29yZCBsZW47CisgICAgICBp ZiAoZmRlLT5pbnN0cnVjdGlvbnMgPj0gZmRlLT5pbnN0cnVjdGlvbnNfZW5kKQorCWdvdG8gaW52 YWxpZDsKICAgICAgIGdldF91bGViMTI4IChsZW4sIGZkZS0+aW5zdHJ1Y3Rpb25zLCBmZGUtPmlu c3RydWN0aW9uc19lbmQpOwogICAgICAgaWYgKChEd2FyZl9Xb3JkKSAoZmRlLT5pbnN0cnVjdGlv bnNfZW5kIC0gZmRlLT5pbnN0cnVjdGlvbnMpIDwgbGVuKQogCXsKKwlpbnZhbGlkOgogCSAgZnJl ZSAoZmRlKTsKIAkgIF9fbGliZHdfc2V0ZXJybm8gKERXQVJGX0VfSU5WQUxJRF9EV0FSRik7CiAJ ICByZXR1cm4gTlVMTDsKZGlmZiAtLWdpdCBhL2xpYmR3L2xpYmR3X2Zvcm0uYyBiL2xpYmR3L2xp YmR3X2Zvcm0uYwppbmRleCBjODNkZmIzOS4uNDAwNDU0NDAgMTAwNjQ0Ci0tLSBhL2xpYmR3L2xp YmR3X2Zvcm0uYworKysgYi9saWJkdy9saWJkd19mb3JtLmMKQEAgLTg4LDYgKzg4LDggQEAgX19s aWJkd19mb3JtX3ZhbF9jb21wdXRlX2xlbiAoc3RydWN0IER3YXJmX0NVICpjdSwgdW5zaWduZWQg aW50IGZvcm0sCiAKICAgICBjYXNlIERXX0ZPUk1fYmxvY2s6CiAgICAgY2FzZSBEV19GT1JNX2V4 cHJsb2M6CisgICAgICBpZiAodmFscCA+PSBlbmRwKQorICAgICAgIGdvdG8gaW52YWxpZDsKICAg ICAgIGdldF91bGViMTI4ICh1MTI4LCB2YWxwLCBlbmRwKTsKICAgICAgIHJlc3VsdCA9IHUxMjgg KyAodmFscCAtIHN0YXJ0cCk7CiAgICAgICBicmVhazsKQEAgLTExMSw2ICsxMTMsOCBAQCBfX2xp YmR3X2Zvcm1fdmFsX2NvbXB1dGVfbGVuIChzdHJ1Y3QgRHdhcmZfQ1UgKmN1LCB1bnNpZ25lZCBp bnQgZm9ybSwKICAgICBjYXNlIERXX0ZPUk1fc3RyeDoKICAgICBjYXNlIERXX0ZPUk1fR05VX2Fk ZHJfaW5kZXg6CiAgICAgY2FzZSBEV19GT1JNX0dOVV9zdHJfaW5kZXg6CisgICAgICBpZiAodmFs cCA+PSBlbmRwKQorICAgICAgIGdvdG8gaW52YWxpZDsKICAgICAgIGdldF91bGViMTI4ICh1MTI4 LCB2YWxwLCBlbmRwKTsKICAgICAgIHJlc3VsdCA9IHZhbHAgLSBzdGFydHA7CiAgICAgICBicmVh azsKQEAgLTExOSw2ICsxMjMsOCBAQCBfX2xpYmR3X2Zvcm1fdmFsX2NvbXB1dGVfbGVuIChzdHJ1 Y3QgRHdhcmZfQ1UgKmN1LCB1bnNpZ25lZCBpbnQgZm9ybSwKICAgICAgIC8qIFRoZSBhbW91bnQg b2YgZGF0YSB0byBza2lwIGluIHRoZSBESUUgaXMgdGhlIHNpemUgb2YgdGhlIGFjdHVhbAogCSBG T1JNIGRhdGEgKHdoaWNoIGlzIF9fbGliZHdfZm9ybV92YWxfbGVuKSBwbHVzIHRoZSBzaXplIG9m IHRoZQogCSB1bGViMTI4IGVuY29kaW5nIHRoYXQgRk9STSAod2hpY2ggaXMgdmFscCAtIHN0YXJ0 cCkuICAqLworICAgICAgaWYgKHZhbHAgPj0gZW5kcCkKKwlnb3RvIGludmFsaWQ7CiAgICAgICBn ZXRfdWxlYjEyOCAodTEyOCwgdmFscCwgZW5kcCk7CiAgICAgICBpZiAoKnZhbHAgPT0gRFdfRk9S TV9pbmRpcmVjdCB8fCAqdmFscCA9PSBEV19GT1JNX2ltcGxpY2l0X2NvbnN0KQogCXJldHVybiAo c2l6ZV90KSAtMTsK --=-DHAps9H/mwgXTVovTZ1g--