Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <26fdb955-10c8-a5d6-07b6-85a4374e7754@oracle.com>
Date:   Fri, 29 Oct 2021 09:15:28 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide
 by zero
Content-Language: en-US
To:     =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>
Cc:     gregkh@linuxfoundation.org, maarten.lankhorst@linux.intel.com,
        mripard@kernel.org, tzimmermann@suse.de, airlied@linux.ie,
        daniel@ffwll.ch, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, ben.davis@arm.com,
        brian.starkey@arm.com, Liviu.Dudau@arm.com,
        Dan Carpenter <dan.carpenter@oracle.com>
References: <1635429437-21718-1-git-send-email-george.kennedy@oracle.com>
 <YXqt46TPL9tUZCL1@intel.com>
From:   George Kennedy <george.kennedy@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <YXqt46TPL9tUZCL1@intel.com>
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?dFlzS1lvaVlucGN0TjA0VzZFQWt6WHdyMVYyUUJrL2Q2K3B5TmlWekErZUow?=
 =?utf-8?B?cHhwRU9HTmcxcWlXRHduU0FqeXhRYWNlZ01MamRHRzgvbnMwaGJuSEpBenYw?=
 =?utf-8?B?bTRvQXAzWUsyQ2pvSEgwS0R3TEFtaURzZlJPRVZTMTNZbzdjaHBZQ0tEbzZ2?=
 =?utf-8?B?akhqRG84MkFxQXlieDVkNW85bjJrTmtwcnhMUllkVGVWZTUwYUVORWlDMzZl?=
 =?utf-8?B?N09WMG5paUl5L3lFR3BmS1hYS3VPVkcvbFJyME9qd0UvMEdHZWdTclFTOGx4?=
 =?utf-8?B?K0dZNzlCZnBmZE9QOWM4ZDdxOEYyUUIzOGRIbEx6WE5PS3lOZmlSeDlmTjdl?=
 =?utf-8?B?UWo1dWk5UXNkNmlCSjZ5K2oweFcyQzZmZmhxb3Z2b1FZY0E5Z2dPb3gvSnNr?=
 =?utf-8?B?aHNjUkVPQTJvSm54aWxNWS9TY3lybzUyV21vU2s4dWdIdmdHTjFGd29Hc2VQ?=
 =?utf-8?B?MU9ETjhDUVJmQjJFcHk5bjJveGxyTVhQOFFEb3lMWlZZSmxkbyt1ZjViTitL?=
 =?utf-8?B?dFZORllhd0lLWitlcXAvUXA4NTA2d2wraVIwN2IzenBXNHFzbnk0TTRFdUdQ?=
 =?utf-8?B?eXFuRE13aFExbzVrdkhyM2toeUpBbTNLZDkvODZjbnFjL0M2dzZmVFloUlAv?=
 =?utf-8?B?ZWtGV0hrREpqQXJRMnBXYzFGanA2VENBclVaVVo4SCtnNUhwSFdQbVZWeC9j?=
 =?utf-8?B?MXVKZ3F3cWVBTjh5cEhqZml2VE9yNVdmS0trSXp5dWppZG5XdWI0M29rYkND?=
 =?utf-8?B?WTVvNHZNRnRCeGZSR0ZnZ2ZqdGVLQ3VSeEE4ZkJncy9yek4vS1FPWC9hVCtt?=
 =?utf-8?B?V0tNN2d5OVdTTUVSNTA0eXpBR0IwN2lUVThobjZ3Y1RDbEVCamJxanlJdjhY?=
 =?utf-8?B?UHF2dnpqeDZvaUMzOEE3dFVRNU5PTCtmYUZGTmlXQ1UxaUU2UXp1ek1ueFVK?=
 =?utf-8?B?dnFyWDNKcmgyOGhsYjlRQjQ1c2g1M1lnak9aWjA1ZFdzc0lIVklFMlNMV3hJ?=
 =?utf-8?B?UWJtZzl1cmQrd1ZTSmFLSE1DdTdFWTFYVUczLzVlLzJSWTh1NjRwdFdNMis3?=
 =?utf-8?B?ZTU5Tmk4NVpyOG9CS28zUWtWMVA4Y3NWNEN3YUZWREp0Yi9QK0NUUmc5Sitx?=
 =?utf-8?B?b0h1YXh3VmdWaXpiWDRzRjZXQkxWODNMSmJ0RjM0OXEzR3VGdS9pTTdKK3Nk?=
 =?utf-8?B?dnNSalJUNXYxNy82WExTcWM2NUlUZU5MKzlzMTNCNHpxeGJwRTJ0eFd3VWRo?=
 =?utf-8?B?SjE2TStZM2hlRmpqQ21NL3g0OHQ3OFBXMnA3LzVFUGxtdWl5OVNWSDh0RVZv?=
 =?utf-8?B?Z3I3blNrdUFpSThMak91SWppVmJwZXladXZSZmNVMVdWVnlFUFFCTTJOV2V2?=
 =?utf-8?B?VythdU1CVVNVOFhIY2E0SjZJb2pXejJDWUdQOFpCRDJKOHdrdCtoSmR2NGtV?=
 =?utf-8?B?UTBKbUgxQ0VhdWxHejU2ODNjMG82RHcrUjZZN2RJdmRKanovcm03WG1oWUJL?=
 =?utf-8?B?N0FNTVBLOGdyc3BJbDVhWE02R1JLVzNVdFIzUGRUN3FrdTFSNmFUZDBVVXFX?=
 =?utf-8?B?aUVSOUNZZTQrcERJVkIvYUlFWnVZTklNQ0VTdWRVUDB2MGhSbzQyc1JrajF5?=
 =?utf-8?B?WVRUMW1WcGZ1SisyOUtiQ21NVUVlNzZOdjJzZmNyNzhqWkJGdVdrOVgrQnNM?=
 =?utf-8?B?eW9FdFpma001bkpIem1rUHhpd3RUbUkwK2NXWDh0SFg2NjdIUVByZzAvbE1u?=
 =?utf-8?B?VGoyYy9sVXFScENaSmthamRSVFVBVmVtcUdwZGVSd0hCaXZ4MjZWNUwwaUdR?=
 =?utf-8?B?aDJIano0SEw5alp6RllUbjFoSG90VkhmU2YvbFZPNzF2RDZxVWRPOXI3Z2l2?=
 =?utf-8?B?QUJnN0hGSFdEMERqNE9acDNZRGFNODkyQ0JCZzdkVnhXeXBWM3IvQ0pZMnZZ?=
 =?utf-8?B?ejNhekk1bHVCMjNwRUlsL0VnYkRKRTZ4S0I3ZTdOeHo2MDZrMVF1YzJVWGhi?=
 =?utf-8?B?S0x1a2drUks4eG5FSmV6YWJ1czllVWlaY0VIZjZtT2JUcDRXelVqUVE3c1VN?=
 =?utf-8?B?NWhpTFpLNUozRFdreXNqU2JOVXF4TXE0d1A3TUptTk5KZDNFVENzMWE5OXVp?=
 =?utf-8?B?QTkwdGJneWI1WVZYaEl5dHZ2R1lIazc3N1hoNTJZZGs2MndLTHRDcEZVMW9L?=
 =?utf-8?B?L2dpWnhmUXJJNUJaNnNXV3Q5cUYvMVFVVUdxMGpTMC8vWGsvZVFuNWdSOFMw?=
 =?utf-8?B?Qjd5aFBaSG1HMEdjZ1YrbDZ1QnpRPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8f7e2243-17b9-416c-f5f6-08d99ade3055
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5192.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Oct 2021 13:15:33.6501
 (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: vTP99qqj1J0pld2IvleNZJPyKkZxbYGVsrDYkC/WQQR8RGtD9aZUoEpNhcwSdT6QCGOISLGOqLVDU36vcvIPyklroRwUjxChrDw2FJ8ZVhc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1857
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10151 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 bulkscore=0 phishscore=0 mlxscore=0 spamscore=0 adultscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2110290079
X-Proofpoint-GUID: AxTiOmWfEiMkh-uAmBf3xDkcZuPuGJMl
X-Proofpoint-ORIG-GUID: AxTiOmWfEiMkh-uAmBf3xDkcZuPuGJMl
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 10/28/2021 10:04 AM, 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.

Adding the authors and reviewer of the patch in question to CC.

94b292b277343190175d39172c903c0c5fb814f1 drm: drm_fourcc: add NV15, 
Q410, Q401 YUV formats

Asking if you have any input on how to deal with hsub and vsub = zero?

The sanity checks of hsub and vsub in the proposed patch are similar to 
other error checking done in framebuffer_check() preceding the proposed 
patch.

Thank you,
George
>
>> +
>>   	for (i = 0; i < info->num_planes; i++) {
>>   		unsigned int width = fb_plane_width(r->width, info, i);
>>   		unsigned int height = fb_plane_height(r->height, info, i);
>> -- 
>> 1.8.3.1