Message-ID: <79d30c1e-2d02-6e0a-6f33-172ab7f91722@oracle.com>
Date:   Thu, 2 Dec 2021 09:17:37 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Subject: Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide
 by zero
Content-Language: en-CA
To:     Jani Nikula <jani.nikula@linux.intel.com>,
        =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        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>
 <YXqt46TPL9tUZCL1@intel.com> <YZdxFvGkBPXrtoQ7@phenom.ffwll.local>
 <YZd2VI820CUGrMjv@intel.com> <YZd8tpDN9lsq0ZbZ@phenom.ffwll.local>
 <87a6i0jk8r.fsf@intel.com> <706003ee-62ca-17a1-f629-6bc010aa5d49@oracle.com>
 <YZ+rSG7VmZ2XbJrf@phenom.ffwll.local>
From:   George Kennedy <george.kennedy@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <YZ+rSG7VmZ2XbJrf@phenom.ffwll.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZW9mSzZDdUhZdUJuUVpJbUtCL1hnY0ZOOVNaRmFvV1Q5RW5UNlB1ZUZ0WGF0?=
 =?utf-8?B?V3FuT1FTaUJqNG5mSWZTZWx4cC9ZRVlmSjd6aXg5R20wOC9XaFZlbSt3M081?=
 =?utf-8?B?dlBtV2FiaERsQTc4UDBDaVBEKzZjTHVWaUNFYjMzWEpBd2c3TmZRM2tCR1F4?=
 =?utf-8?B?cmpnMFQ4dU5YeEZOS2VZQitOU1poR2pueEJOTCttVVN3U3RsR2hyd3k5Q3p5?=
 =?utf-8?B?WVBRb0d4Q1FCVDBPaFBHdkxGZmxQbS9lL0FkYWtUc0I2TEkvOGhoTmlQWVoz?=
 =?utf-8?B?MGRQaEE5V2dISytmdThGN1pzd0Y3NmRCc09QMm5NNHNhaStBeFFVL2dlbUZZ?=
 =?utf-8?B?SDFmaVVqUWVRdnRTeXlkQW1adVdXdWV3Q3piT0c5OGVxRWIzM1FVY3h4RlRH?=
 =?utf-8?B?VW5jclVFaHFCZHIxY3JqQ3BKV0o5UHlTZjBxcUNmZjE3ZzhqaFdFNFdnNTdB?=
 =?utf-8?B?cUJkaVg1T2JtMVZDelg2YThtaWgzb0g5NUlKKzM0UDE0cHNPVGRSK3hkUEdW?=
 =?utf-8?B?b0wzTmo3SElzaGM1cnh4Q245TjJtSzUzMmtNWFlFa0tqK3J3VFpYYWVkeUNp?=
 =?utf-8?B?ZFRILzZMNmx2RjI2d0hYYXRxTEVZRWxpL253R01mL2paZjVwSXlaS3hreWcw?=
 =?utf-8?B?cmxNaEhZQ0NPaDFzckdxRWQzQnNPeWNnTlNOYmFqWGdock9QbkQ2d0krZ0F4?=
 =?utf-8?B?Z0tneDVRZjRpY2NZSkRDUnJxL0RjdFpCa0c5eW04bXVSVGdyaVh3aEFaWHpM?=
 =?utf-8?B?RFNSMVQ2c3A0TDVXRENhZkFOZGFyRG9ZRVZLNDRGR01vSzIvZ3J3SFkyZ0Ns?=
 =?utf-8?B?emlMdSsvOXJjTXRVVU5oNitWMEpRZVVRNVdZRTZrcFdrQ1ZzZ3pHMnhNS2Iw?=
 =?utf-8?B?Y0VZTG9LQkViMXJ5U1BEVXRQUmxBMnJUNVF2SW9wS21Tenh2UURld255M0RS?=
 =?utf-8?B?Y0J3M3BZcnBsME0rYVk1S0RGZDVYZnBhODBVWG9KQjB6dDhzYy94NnF6U1Nk?=
 =?utf-8?B?K1loQnlKS0pkRTdHZHB4T0g1NWZPcDR5VjFydjNTV2RnWkJlaTlUZnpCTTcy?=
 =?utf-8?B?SENUOHhtaXhIeWFHTFg0c1JUVTUrV05PUFFVYi9ycGxsQmxhcWdJUHdEL3dV?=
 =?utf-8?B?cEkwdVQwVWprVmw4d2JvTkoyZU9CTzNkVktBQkUzOG9rKzNPeTlaSHFxTlZ5?=
 =?utf-8?B?QVlCMnptTGRQbE1kaHZNdGpWK3ZsakhrY1dFL2tRM3FDbk5YR1p5eUV5cC9p?=
 =?utf-8?B?RGZFTUQzS1lwUStLa1lmZ2w5NjJuYmpmMzFOWEllOHFHOWx0bU0yRm4rY1lr?=
 =?utf-8?B?NTFrNHdBSThlbXdEbEZDdndZODdPQ1Q2eGZCZEVPSThNRVdNRmpyTThUTWpp?=
 =?utf-8?B?c25TM0tTcVBhWStxdXErYXZhbkkvSDBrOE41NDV0THBlQnZMTzNqYzZ6VUZF?=
 =?utf-8?B?U1hjV0w3S1BkWHhKNDlsTVh6bGRWek52QmRzd2J6Ykw2K3gvd3QxbzY3ay9o?=
 =?utf-8?B?TEhxb2tBamFzQWJZWmV3Zlhub0tIbkRKT3RmdmpHTE14dE5Hb0U0dWZJaEpw?=
 =?utf-8?B?Rmxzcys4RVYwV2dKUU4vUHF2RW1aeHJ4TEx6NjBON3V2VVAycW9teld1YnEy?=
 =?utf-8?B?dWVudmNIcWJwa1c1RG1EQ3lscnF4UHVnVWRCR0Fid0wxcGpWZndreE5Da3hz?=
 =?utf-8?B?Rzk0blhqdHVRbEhNOGJKL05TdDMzUUx0U1djZmtjMUYwb3R0Kyt2aDdQWTE4?=
 =?utf-8?B?QzVtVU9HQTFZZDBvMlRLNmNQT2tkSDk4YUJybk9uUHU0RE1SMGJEZnV4NGU5?=
 =?utf-8?B?Q0QyWjBmUkVrQVFGTFNxbnlzeitrNzBPK3R6c2JhVUFLVHl3L0ZweXRta1BO?=
 =?utf-8?B?V0xka1hwV0Q2bnExRmpkU1NmQ1R5OUxHdCtiTU5IRzdUVnp4NHJhZFFBZWha?=
 =?utf-8?B?WFRtUTRFUUhsVTRBZTg3ZzF4UlBmRjUxbGs3a294dnlWKzZYUkduWkY1K0dG?=
 =?utf-8?B?QkVDQ3p2TU5tOHI4U2o2RFpTQytFWk1raTkzT0VKVzZRSHRaZGczVmFpOGdQ?=
 =?utf-8?B?YThpb20reCtXbnNwYUZVSG83VEFyT3JOUlNGM0dEY0psbWlhSktYWEREUzlV?=
 =?utf-8?B?ejQ5bHIzZC9yaS9jejJuRUF6elhOS2t4R0NBcitqc2lqRVdWL3lhMUNOb0RP?=
 =?utf-8?B?dzNSdXgzbXphK0ZjbmlMRnQra0x3UGdFb2JCQ1pod0VRK1BPZm9UaTZ3ZWNl?=
 =?utf-8?B?ZXgrK1hKS2tnUjRkWll2VzVQV2VBPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 251f9d7b-929d-40b6-f9b9-08d9b59e8071
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5192.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Dec 2021 14:17:41.7852
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: COeAvnznYjM3kbEQT3dd5pc2t1tI4vyjAH6D2ulicfM1wuskeUzY0CoKvajab+12/rqT7AwhYYERoIWYRaiYA8VpSxM+iHnsIr5gSzWDDx0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR10MB3202
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10185 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 adultscore=0
 bulkscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112020093
X-Proofpoint-ORIG-GUID: e8KIbyNqfSL5XRaGtb1x0ftLx2ctwcqK
X-Proofpoint-GUID: e8KIbyNqfSL5XRaGtb1x0ftLx2ctwcqK
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11/25/2021 10:27 AM, Daniel Vetter wrote:
> 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 <daniel@ffwll.ch> 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 <george.kennedy@oracle.com>
>>>>>>>> ---
>>>>>>>>    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.
In the meantime, should a bugzilla bug be opened to track the issue? 
 From this thread it does not seem as though there is a drm selftest in 
the works.

Thanks,
George
> -Daniel