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: <c73402dd-ec3f-4df7-6a16-36fb276a50f4@oracle.com>
Date:   Tue, 26 Oct 2021 09:36:29 -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:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sam Ravnborg <sam@ravnborg.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <1635188490-15082-1-git-send-email-george.kennedy@oracle.com>
 <YXcAbXJfg/jKCo5h@kroah.com>
 <0ddb1c19-64b0-4117-7a92-c3d2fcddfdcf@oracle.com>
 <CAMuHMdWcU7wy1dxa_MXuHoUTWebvq6ShP820Bn_v+8tx7-TEdg@mail.gmail.com>
From:   George Kennedy <george.kennedy@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CAMuHMdWcU7wy1dxa_MXuHoUTWebvq6ShP820Bn_v+8tx7-TEdg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NTlFYWpTWnpVSmx5K3p2THBPNm4xUFVQeTV3ODBuUmJZbW1HUjYyVkIybjdU?=
 =?utf-8?B?NDdqbnFNNGpKYlg1bjZaKzFNUGpzcFg4MFhFbjJHOXQ1TWNHRjc0aEhBdmlo?=
 =?utf-8?B?cWxTRE5uenlDcUZ0TWJETDliTi9RSVF6akhTZ1Y2cGNuZ1kxT1JncGNrV3R1?=
 =?utf-8?B?aHJ0TE1mMVVqQ21tQUtVd3VwQ0xuQ0ErdExOQVJhN1BkZ0NxbFVoVkg1eEVE?=
 =?utf-8?B?YncyQ25xcnFJaWFENjNDTk01T01RcW9xUHVwQ3NCaEhYK1E5Vm9HU056WnJB?=
 =?utf-8?B?b1A3K3FPZWo5QmVKVzI3bm00QUNFWmhCcVkzR0w2YmdCNjIya200QXNPbVhr?=
 =?utf-8?B?TU5BSGJLblBzOWgwY0dJdWdGbEdYTEFhMUY4VkJJRXVhd09zUkp3Vmh4L3dU?=
 =?utf-8?B?VytVaDg2cnRtNUdJaDZxZEp3YmhoZmdJdVBrNU5USzBtbVpTUUl3OFV0T0dZ?=
 =?utf-8?B?OWVuMC9ocEVmSnhsdkp6cEk5RzFZNCs3dys3b0dlQlVxZjFmbWowUnRVY0dI?=
 =?utf-8?B?VGNCM0tjL2trZWl3TU9pVDdGc3M0YjFWa0tETitYYmcyVXNoWkMzaGFKaG9G?=
 =?utf-8?B?NEdybW5QajVpQTBVRWJ3TnlTYmVoUm9hWWhqQnluMkJVSVdSWEpQVS83cGNI?=
 =?utf-8?B?Rzc5R3B4VFV4RDFlWERJQi9kVG14WDdpVzFZKytVYnlWNTB4eEM1b0Z1NXVH?=
 =?utf-8?B?Myt3WDlWOUpaVHhFeFN2bjBHeVh6RDlIN0hYMTJTUkZxWnB6MDFvcE9jaWdr?=
 =?utf-8?B?R2NSOXJOUXZTdldkVGdKR28yclVtdHZmZ1RHeWV5amdmREtQY3g4N1N2N29Z?=
 =?utf-8?B?NlZLdGpERVlsbEZhUTV5anRtZUdrV3YvOXRsdktTMFJheUZBNGtqcTN6ckVu?=
 =?utf-8?B?bVZBc2lXOGtLcVRFbTQzazNsT09HRUwyNmNyOElPSFB3a0tlU2UxazVSNXZP?=
 =?utf-8?B?QzU3ZlZHOVQ5eDBnNDduYTJsYmtxQXhPbEh4empmS1ZPNUM0ZjJqdXRCWUR4?=
 =?utf-8?B?bUV3Q01LeXB6djhzWGFYYXlaYlZvcDBWVS9oOGkrS2hlSXdFU0t6b0Y4blV4?=
 =?utf-8?B?a0tCMU85SjIxcXNIQUtibFZ4VW5MMW9NaWdMd01MbzJhaWZrZEk0OUMrdXRH?=
 =?utf-8?B?d1VmQ2QyRUVKTnJxdnVoVjBleXl6UHN2SmlabkJSOXJrTDhzUUFPdmZPU0RR?=
 =?utf-8?B?aXQ0c0VPdUN0YnRwc0lKbndMaEtJdGwxSWVSaUhFalVEQVVtVk4zZVBIVlJN?=
 =?utf-8?B?SmUyQVlzQVR5SVJ4emF0TFpKVjN2ZFpndDRLVVdyVkFHaitTZmNTTmwzUkV2?=
 =?utf-8?B?cjZWaCtsWDZ4U2wxT28zaU1YWG9KbFRXNWNjMnZOcXpQY3BzT3Z6VStlcHF5?=
 =?utf-8?B?enZ3YmVBMUpHN1VaQTZuZTdjeWJ4YlBaS052ZnpNcnh1NGVUY2xXSUZWUE80?=
 =?utf-8?B?WmxMeWJQa2NxUGEvakh0cmtoZDA2eXN2dW1WakU4RldCM0ZSLzZnSnhPK2M5?=
 =?utf-8?B?YUttYW1mV05pOGJnMmVITTNGa3BubGkyamdTclJWeXRpell2RzUzczhDVE5q?=
 =?utf-8?B?VjhyVVBZUit6N2xwS2hQSmVtemcxeU9QRFdldDNpSndRM0VvUEpucVZDWUV2?=
 =?utf-8?B?Mkx3SENFQy96Wjc2eUx1cTF6SUhaSW90cG9vZkZmeGt1ZHZBblFjNWVRYUdi?=
 =?utf-8?B?UGRHVCtsanVxUTlKWWY3RFRlQTFFVTd6U2o5YjZia1lFSnZIdUI5NEVNNXFx?=
 =?utf-8?B?Z2RBcXpTeFhlQVU1YWxJMFRpY1UrWm5RUEhQbkQ3SHhGVWpKMDM5djRHVzlo?=
 =?utf-8?B?cHNnS1phVCtRa3FCVjlETlJPdFNZN3pyajA2cmJhUXkvSXFmUkpiTVE0R1J0?=
 =?utf-8?B?cGJWTU5Sc3hoVHVQRHFMeHFqb0VWMDEzM3Y4Q3hvN0JBL0JQcU5Pb2x2QldI?=
 =?utf-8?B?WWZHOURabzJUSDJPdTNzNjZDYjdUUXkwenNhdEJFUkkwMkExcFpKZjY2QkY3?=
 =?utf-8?B?SWJFbkh4Q2d5Wm0zNG5HZ1dWSWwzS3hoS2UzQW5OdHluRVlBY2FBUDRKb1J1?=
 =?utf-8?B?ZmErSWxDTXIxeFNycGZwM0tpdWFoSWxLemNTUldld2FWMVFFdnowcXRQU09q?=
 =?utf-8?B?SDZqWGxZbHJXUE1Welo1MEpYK3o5TkM1Uzd6ck02RmY4NHd0ZnZYWW1qcUlB?=
 =?utf-8?B?YkZEaEpnT1VkQlNlUnBGY3I0UlRqaEVpcGVTalpaNUNodHZXb1dKblJQZ05s?=
 =?utf-8?B?NzdQRHFmRlFDRW5hWXlBVE9Kbzh3PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f596f6ab-3f90-4b42-eef1-08d99885a3d9
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5192.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2021 13:36:39.9962
 (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: /+2Z57inQH1HEuu8rN+sSurQrQVyaj6HRz/UIzfuUxW3sYnf6o/r3y79BwL141GowKpEiqGw5SFT2eHtBLITVly9ale4qBYJlRNw/dIKFaE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR10MB3281
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10148 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-2110260078
X-Proofpoint-ORIG-GUID: n2rutvF6dnefhLBpBqi1Lt-ALV4msb_T
X-Proofpoint-GUID: n2rutvF6dnefhLBpBqi1Lt-ALV4msb_T
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Geert,

On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
> Hi George,
>
> On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
> <george.kennedy@oracle.com> wrote:
>> 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;
> This is not correct: fbdev drivers should round up invalid values,
> and only return an error if rounding up cannot yield a valid value.

What default value would you recommend? Here are examples of some of the 
possible cirrusfb pixclock values:
40000: 25MHz
20000: 50Mhz
12500: 80Mhz

We can plug in a default value, but I believe it is just covering up the 
fact that an incorrect value has been copied in.

I would think we would want to keep this driver consistent with the 
other fb drivers that return failure with the incorrect value.

Thank you,
George
>
>>> 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.
> Indeed, several drivers are not following the rounding rules.
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds