Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B1FBDC433F5 for ; Thu, 25 Nov 2021 15:35:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1356127AbhKYPin (ORCPT ); Thu, 25 Nov 2021 10:38:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46396 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1356056AbhKYPgk (ORCPT ); Thu, 25 Nov 2021 10:36:40 -0500 Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D6318C061785 for ; Thu, 25 Nov 2021 07:27:07 -0800 (PST) Received: by mail-wr1-x431.google.com with SMTP id v11so12402780wrw.10 for ; Thu, 25 Nov 2021 07:27:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=aJGhzRapkjxnSquRFQKdZiwXU/SjGJ7w38ExsLPAS2A=; b=EO/nVb5iod4bsDoCm3UD2u7+zCp3ZRp8SwHsRDdIsKKqoDGtKPRa+7aZkytV+FF3c2 be+ykRZhM8B9uRRTgX8Z3uXfHvMpMO/JvQ8p9JNcE7b4e/lJrxf9fE3RGwto8Uqn2nSE yibEAJiGgv653/eg+oCa1mWUq8ZxfDkeILZcw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=aJGhzRapkjxnSquRFQKdZiwXU/SjGJ7w38ExsLPAS2A=; b=4THNko5nTOjiqmlC6ejdLPqcshue3OSJDgrBSD0jx4tDEc32G+s1e20jaMY6n1IpyF tCKDmrrtSUHi60NV2fodfYwcWXCkoSJzH3wrQ0TTUIgwbWvB99lUB3STC8j+V35O6V7p W2Qj5I1b96cpFwckTS24B1/U1tdaso+NvIeft1DtRRboQ8I2xboJb8jFCULQVs5f2bKe PKvawBRWuDkvDvwYYTR+DfHqHOqYrSn9NNNOPovyEDYXvTUzzXId1dwolmS/KASXMCCm hhxPyYi8w6onjbwIJfXCRLtK53lVEzoCHWyh0MXq6PKqLnmsAk0SEjBCqxyaqlPxFBgU s/Mw== X-Gm-Message-State: AOAM533BIo2OFdeJ8VkFtEkFMl/74iFDj28vnNm+trOWbTNbqTmGHIHb zAqkqooG3QiyjzNG928XYTIJD1Sue6/6fw== X-Google-Smtp-Source: ABdhPJxsHddb9FpqPS3lP+icqHfabOZVXFoAOCQFOfbOxipWKNbUFgEs4eInNop8/mNtSbgGGUAP/w== X-Received: by 2002:a5d:55d1:: with SMTP id i17mr7418506wrw.618.1637854026426; Thu, 25 Nov 2021 07:27:06 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id b14sm4077017wrd.24.2021.11.25.07.27.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Nov 2021 07:27:05 -0800 (PST) Date: Thu, 25 Nov 2021 16:27:04 +0100 From: Daniel Vetter To: George Kennedy Cc: Jani Nikula , Daniel Vetter , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , airlied@linux.ie, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, tzimmermann@suse.de Subject: Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero Message-ID: Mail-Followup-To: George Kennedy , Jani Nikula , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , airlied@linux.ie, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, tzimmermann@suse.de References: <1635429437-21718-1-git-send-email-george.kennedy@oracle.com> <87a6i0jk8r.fsf@intel.com> <706003ee-62ca-17a1-f629-6bc010aa5d49@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <706003ee-62ca-17a1-f629-6bc010aa5d49@oracle.com> X-Operating-System: Linux phenom 5.10.0-8-amd64 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 22, 2021 at 10:29:05AM -0500, George Kennedy wrote: > > > On 11/19/2021 9:25 AM, Jani Nikula wrote: > > On Fri, 19 Nov 2021, Daniel Vetter wrote: > > > On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrj?l? wrote: > > > > On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote: > > > > > On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrj?l? wrote: > > > > > > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote: > > > > > > > Do a sanity check on struct drm_format_info hsub and vsub values to > > > > > > > avoid divide by zero. > > > > > > > > > > > > > > Syzkaller reported a divide error in framebuffer_check() when the > > > > > > > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via > > > > > > > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for > > > > > > > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". > > > > > > > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses > > > > > > > vsub as a divisor. These divisors need to be sanity checked for > > > > > > > zero before use. > > > > > > > > > > > > > > divide error: 0000 [#1] SMP KASAN NOPTI > > > > > > > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 > > > > > > > Hardware name: Red Hat KVM, BIOS 1.13.0-2 > > > > > > > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] > > > > > > > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 > > > > > > > drivers/gpu/drm/drm_framebuffer.c:317 > > > > > > > > > > > > > > Call Trace: > > > > > > > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 > > > > > > > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 > > > > > > > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 > > > > > > > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 > > > > > > > vfs_ioctl fs/ioctl.c:51 [inline] > > > > > > > __do_sys_ioctl fs/ioctl.c:874 [inline] > > > > > > > __se_sys_ioctl fs/ioctl.c:860 [inline] > > > > > > > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 > > > > > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > > > > > > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 > > > > > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > > > > > > > > > > > Signed-off-by: George Kennedy > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ > > > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > > > > > index 07f5abc..a146e4b 100644 > > > > > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > > > > > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, > > > > > > > /* now let the driver pick its own format info */ > > > > > > > info = drm_get_format_info(dev, r); > > > > > > > + if (info->hsub == 0) { > > > > > > > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + > > > > > > > + if (info->vsub == 0) { > > > > > > > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > Looks like duct tape to me. I think we need to either fix those formats > > > > > > to have valid format info, or just revert the whole patch that added such > > > > > > broken things. > > > > > Yeah maybe even a compile-time check of the format table(s) to validate > > > > > them properly and scream ... Or at least a selftest. > > > > I really wish C had (even very limited) compile time evaluation > > > > so one could actually loop over arrays like at compile time to > > > > check each element. As it stands you either have to check each > > > > array element by hand, or you do some cpp macro horrors to > > > > pretend you're iterating the array. > > > Python preprocess or so seems to be the usual answer, and that then just > > > generates the C table after it's all checked. > > > > > > Or a post-processor which fishes the table out from the .o (or just links > > > against it). > > > > > > But yeah doing this in cpp isn't going to work, aside from it'd be really > > > ugly. > > Kbuild does have support for hostprogs which are typically used in the > > build. The obvious idea is to use that for code generation, but it would > > also be interesting to see how that could be used for compile-time > > evaluation of sorts. Kind of like compile-time selftests? And, of > > course, how badly that would be frowned upon. > > > > git grep says there are only four hostprogs users in drivers/, so it > > certainly isn't a popularity contest winner. (One of them is > > "mkregtable" in radeon.) > > So, can someone suggest a fix? A cpp type of approach does not seem > feasible. > > Adding the sanity checks that are in the patch, which are similar to the > sanity checks preceding them in framebuffer_check(), along with a self-test > that ran through all the table entries, might address all the concerns > brought up in this thread. drm selftest sounds like a reasonable approach to me. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch