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: <0ddb1c19-64b0-4117-7a92-c3d2fcddfdcf@oracle.com>
Date:   Mon, 25 Oct 2021 15:33:43 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] video: fbdev: cirrusfb: check pixclock to avoid divide by
 zero
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     tzimmermann@suse.de, sam@ravnborg.org,
        dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1635188490-15082-1-git-send-email-george.kennedy@oracle.com>
 <YXcAbXJfg/jKCo5h@kroah.com>
From:   George Kennedy <george.kennedy@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <YXcAbXJfg/jKCo5h@kroah.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?R3M5L2EzaHhNMVM0VEdwRHRPb1pwcS9iYWdTU0U0aTN3N3VkdnBuaHpQcUdN?=
 =?utf-8?B?dlpqSXBwcnM1ZUF3MElxaTUrTEtzaDM5QmwvVW5xV0p6OFVEdG40U2NaNVY1?=
 =?utf-8?B?QzZqQVp1Vy9OdW5IUzNTLzI3TTRKMzF6SXdvQkRFdmQwWmk4bGtDV3B3OWZs?=
 =?utf-8?B?MU1JV1VkNFh1N0V6UnRUKzNKZXJpbG9BZ1M4L3dWb3I4K3NXTmRzZUVMTitj?=
 =?utf-8?B?ZVdpQVUwaUpEMmFIb1piWUpVUFA0R3Yra3M4dDF1SWtQaWl0dDhRR0FOMHQ3?=
 =?utf-8?B?L2RJVTZ4dHZjczA2TzBoTVBKMC9SZTJLRUpRL09INkk1WjJ0b1hLYUpSRFMw?=
 =?utf-8?B?Zksxem9wcGxCMFZTOVR4YnBZRnZsVmlmaldIR0p4ZTZCenNMNlpHRzd3Sjl3?=
 =?utf-8?B?a3FiVzlyNmRLK1V4dlhjdWNwbFN4NEVRRmVLT0FuSkRRek9naXZLK2dHdUQy?=
 =?utf-8?B?b0Y3cXI3TVhySHVYc3dvaTdSaVhBVGFQTW9qa0hSNW9jZDZSZjN4RVNqMEVG?=
 =?utf-8?B?VUc0V3YrSC9IWHBESDFIR3o4cmxPK2pKcU5iN2JqTEMvM1krNnRaK1NsYmxl?=
 =?utf-8?B?NFo5MnIrUW9pQXFnV0NQS0p6YzF1VGI5M0tmYnVpQXFHbHZUck9ES3VQQXVn?=
 =?utf-8?B?cEJJckJsZEpjZjkwOFlONnBsM1J3M0NvV2dyRXQwTlpsbHBhR09BdTVFQmd4?=
 =?utf-8?B?YlJyaldFT1M1L0wxMUY0cVlFcTk2RGx3QjR2aVZBN2tSY2pDZjBQeVRXeDFp?=
 =?utf-8?B?dXZFSGs4RUhUdU5CMWZoT25ma3Z3WFBnTFM1OE1hM1BaeEc2NUMyMTdMNVpF?=
 =?utf-8?B?K3YvTFBjbHZiaExTeTl4cStrd01CNUVhaTBoM3VjZDFtdWRuamlCVkQ2a1BY?=
 =?utf-8?B?ZGdGTTYxQnVRZVNLalluN3RYSkdaNU5uQVdjZXlINVdKVWJTVmluNEtOQTFl?=
 =?utf-8?B?ZzNmM2hXZlFkQmI2M0xLanRwNkptL2ROVE94NHV6MDVXNVowRmVRT2twUDBh?=
 =?utf-8?B?blIzK0NBM3EvaDd6VGtKVzJkN2tITFB4TUJ0T1lVVm9meVJCdWwxS1hmdW5V?=
 =?utf-8?B?VFdFRER1VU5jZVJObHZhMXZxa3JHcDJwK1MveHk2bUxUNkxaMk5sb2RpZ0Jh?=
 =?utf-8?B?SHgzNWVEMFdRUUhsMC8yNmNkNlMyVUdkYVl0VFZkUFF2Z0IvV2FoWGpzK3Fj?=
 =?utf-8?B?aTFheC9DeXdJcHpkRHRxQmJzZ1BTVnJhbFlPbFF5bWtDTTdhNjRmQmJsdHFq?=
 =?utf-8?B?SkRtb0UyajlUNFZ2UUxVZlN3eWt5TkFnSGxCUUI3em5FLzFtQVFKSnRxM1FO?=
 =?utf-8?B?QW9hTWNOdE9IUWQwcVg0WGZHSXM2WGJ6QitqeGI2MllBZHVjbFFDbTkzWENC?=
 =?utf-8?B?SGJrcFdad2d4ZzZ4aElqQzF6ZU9sYmtLYkx0WU93b25YbEEzYXB1aEtGMm1N?=
 =?utf-8?B?TVVJZXFua2cwTEVXa3JtN3J4S3B5Wkw3eXltV1Zydmw4ZHJFL29nQWVwd2Vj?=
 =?utf-8?B?Qmd0MzZjNWtGd0ZzcTlkcVhKbVNXOVpLb1hCSXVta0k5b2F5Mm40Rk9uTm1n?=
 =?utf-8?B?cWxEZUgvYXErcTJ3RTZMekVlVTd6YkNMM2NIcXhrV3NyM1NnbTFESkppanZL?=
 =?utf-8?B?Yk1oak9vQnEzNnp3b2pKZXcvR1J5MGdscVE3a0N0QzZxNzdYbjFWL2pLQ2Ez?=
 =?utf-8?B?ZG4zOVJSOEs5VlJKWkU3MXg3YTg0NUdYdkQ3Yng5QWZRUS9XWnhKcnJibTVY?=
 =?utf-8?B?Qy9uY1RmejRLQkVzekpXTUUzalJBUEVRRGpkSXQzZ2M0aVNUSjU3RzdsL1hw?=
 =?utf-8?B?TS8yU3Q2ME1ENHBLSDZ1RjRWMDBsT1JSbXNoUjZsdWpadjlpSEpIQkZhWGVa?=
 =?utf-8?B?RzNTWEZ1TVZWRFlicEptN3FvcEh0d3MwTEZWSmg3MjZzZVQwMXc4ZU9XVGI5?=
 =?utf-8?B?VnZDOXNWMkxVcCtZYWNoSlFBMXV0cXdyVHVoQ0EvSGtXb2poaWRIZzhxWjJW?=
 =?utf-8?B?SGp0NVlrTVdCNTdBV28ycWRhVURaQXVib3RuREJRY1RsR2pwZUp5RnV4SVBa?=
 =?utf-8?B?b01WZUd0azk4cTFXSTJ5LzFrdkNjYkwzdHpqeTlBSTdSMUhUS3Rqbnd0NzNI?=
 =?utf-8?B?bWFYSEt4SjhnTnZsb1M2d1pXNVB5S29tMWdlQ1o0a3hhdnRoblNUd3d4Z3pU?=
 =?utf-8?B?VVJEMFFpZjZ4NThhbGNmSnY3VVQ1SW9SQnNmeXBrZjFxMmtYVmxqVi9zcXVY?=
 =?utf-8?B?UERQMG4waEV0UWV3WXk1U3dGQzJnPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 18e9a1e0-8e02-435d-69f2-08d997ee5bc2
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5192.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2021 19:33:44.8119
 (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: 5GeUMXHQglKYQdCggWaZSpiEgShIQw5K4UiweOw21p1p0T8LFJWUi3DbpIBzOMCXpsi2n02vUqVh52VVqv64ibnifEU0Fs6i4/AIgMT6NPA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5224
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10148 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 suspectscore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2110250113
X-Proofpoint-ORIG-GUID: Ldxuk6w6NSXvSPc3tf4q1-VAX6E36Ukq
X-Proofpoint-GUID: Ldxuk6w6NSXvSPc3tf4q1-VAX6E36Ukq
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 10/25/2021 3:07 PM, Greg KH wrote:
> On Mon, Oct 25, 2021 at 02:01:30PM -0500, George Kennedy wrote:
>> Do a sanity check on pixclock value before using it as a divisor.
>>
>> Syzkaller reported a divide error in cirrusfb_check_pixclock.
>>
>> divide error: 0000 [#1] SMP KASAN PTI
>> CPU: 0 PID: 14938 Comm: cirrusfb_test Not tainted 5.15.0-rc6 #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2
>> RIP: 0010:cirrusfb_check_var+0x6f1/0x1260
>>
>> Call Trace:
>>   fb_set_var+0x398/0xf90
>>   do_fb_ioctl+0x4b8/0x6f0
>>   fb_ioctl+0xeb/0x130
>>   __x64_sys_ioctl+0x19d/0x220
>>   do_syscall_64+0x3a/0x80
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
>> ---
>>   drivers/video/fbdev/cirrusfb.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c
>> index 93802ab..099ddcb 100644
>> --- a/drivers/video/fbdev/cirrusfb.c
>> +++ b/drivers/video/fbdev/cirrusfb.c
>> @@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,
>>   	struct cirrusfb_info *cinfo = info->par;
>>   	unsigned maxclockidx = var->bits_per_pixel >> 3;
>>   
>> +	if (!var->pixclock)
>> +		return -EINVAL;
> Shouldn't you be checking further up the call chain where this got set
> to 0?

The same pixclock check is done in these fb drivers:

arch/arm/mach-rpc/include/mach/acornfb.h:    if (!var->pixclock)
drivers/video/fbdev/asiliantfb.c:    if (!var->pixclock)
drivers/video/fbdev/clps711x-fb.c:    if (!var->pixclock)
drivers/video/fbdev/core/fbmon.c:    if (!var->pixclock)
drivers/video/fbdev/core/modedb.c:    if (!var->pixclock)
drivers/video/fbdev/cirrusfb.c:    if (!var->pixclock)
drivers/video/fbdev/kyro/fbdev.c:    if (!var->pixclock)
drivers/video/fbdev/riva/fbdev.c:    if (!var->pixclock)
drivers/video/fbdev/uvesafb.c:    if (!var->pixclock)

>
> What logic allows this to be a valid value?  What about all other fb
> drivers?

The "check_var" function, which is set into the ".fb_check_var" element 
of the fb_ops struct, should do the check, but in the case of cirrusfb, 
that is not being done.

All this patch does is add the same pixclock check that the other above 
fb drivers do.

BTW, this patch should also go into stable (specifically, 5.4.y).

Thank you,
George
>
> thanks,
>
> greg k-h