From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Paalanen Subject: Re: [PATCH 1/8] drm/fb: More paranoia in addfb checks Date: Fri, 15 Nov 2019 12:49:44 +0200 Message-ID: <20191115124944.25e31d63@eldfell.localdomain> References: <20191115092120.4445-1-daniel.vetter@ffwll.ch> <20191115092120.4445-2-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1761834773==" Return-path: In-Reply-To: <20191115092120.4445-2-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Daniel Vetter , Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org --===============1761834773== Content-Type: multipart/signed; micalg=pgp-sha256; boundary="Sig_/isEOVCvveAkT4r=oyK/C6kr"; protocol="application/pgp-signature" --Sig_/isEOVCvveAkT4r=oyK/C6kr Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Fri, 15 Nov 2019 10:21:13 +0100 Daniel Vetter wrote: > - Our limit is uint32_t, make that explicit. >=20 > - Untangle the one overflow check, I think (but not sure) that with > all three together you could overflow the uint64_t and it'd look > cool again. Hence two steps. Also go with the more common (and imo > safer approach) of reducing the range we accept, instead of trying > to compute the overflow in high enough precision. >=20 > - The above would blow up if we get a 0 pitches, so check for that > too, but only if block_size is a thing. >=20 > Cc: Pekka Paalanen > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) >=20 > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_fram= ebuffer.c > index 57564318ceea..3141c6ed6dd2 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -214,15 +214,20 @@ static int framebuffer_check(struct drm_device *dev, > return -EINVAL; > } > =20 > - if (min_pitch > UINT_MAX) > + if (min_pitch > U8_MAX) This looks odd, but I don't know what min_pitch is or why it should be limited to 255(?). What's with the U8, should it not be U32? > return -ERANGE; > =20 > - if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX) > - return -ERANGE; > + if (block_size) { > + if (r->pitches[i] < min_pitch) { > + DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); > + return -EINVAL; > + } > =20 > - if (block_size && r->pitches[i] < min_pitch) { > - DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); > - return -EINVAL; > + if (height > U8_MAX / r->pitches[i]) > + return -ERANGE; > + > + if (r->offsets[i] > U8_MAX / r->pitches[i] - height) Aside from the U8 again, this looks strange too. You want to check that offset + height * pitch does not exceed MAX? Wouldn't that be height > (MAX - offset) / pitch for bad? If offset cannot be negative, this could also replace height > U8_MAX / r->pitches[i]. > + return -ERANGE; > } > =20 > if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) { Thanks, pq --Sig_/isEOVCvveAkT4r=oyK/C6kr Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAl3OgsgACgkQI1/ltBGq qqfu1A/+LMeAvBfMrsDU9bqy6FW1FEv938rdKt/NjWPl99rfWQbilZYxlW4KITru lJ8ErQ6IUhVB1EDGQrQtkbqk9s/Q4a3qY8L41pYJ2e2TedS+5Tcu3emYhweKAkX2 qJUbbfO2IoRMG09F2Xow1NkND1vOWZGOZbiZCGxihXTIzOiFu0MRvCqpUXyGlOCC yTEF1NKofNS19s9rIiRAZGshwtMNXak52+sM97ls3NhnGhsEFuilCTe0jD6IBfVi UYpKpRRtVMiG4Q2gYq0lgHsWl7tYaFKKLZq0LguVZAIGfPDJdmjj7bPSpnVEYWaq vAVyKyN1xKJbSvd0nph3c6KZqRyLw3EiJ3UyTUjHgDNPLxBCgtdTAr4O44a7MqnV mdJNf2i/fTcKe/83jGvW9kiC7n0Muce4nM76+brxtkbI57OB0C2o+F61mMMj/V/m 5Uf5gJBuwxIEHJEdTr0m3ohF8wAZEp7m34j//hxhmylnTcDRNBqHTgRKSn+AkIEP sJD6z/e7E69a6kPrvM6xYkhXag+zEOXQ17qQMWijl1sMuiEmZxzk33/EVLGR42ES prz2hXlOX36ZfPAEatspOotut5+rt9LzLaQhz37DAwQEDkwXkWSqu+SJZ/YlJ3eH B2BsrtWuMgLV5Xs/WEITmOhoZeOzAgxzCJeBCcb8aC/B8y/VAp0= =CoGY -----END PGP SIGNATURE----- --Sig_/isEOVCvveAkT4r=oyK/C6kr-- --===============1761834773== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1761834773==--