lkml.org 
[lkml]   [2016]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
From
Date
On 09.03.2016 15:46, Alexander Popov wrote:
> On 16.02.2016 18:18, Peter Jones wrote:
>> On Tue, Feb 16, 2016 at 01:49:18PM +0000, Matt Fleming wrote:
>>> [ Including Peter, the efifb maintainer. Original email is here,
>>>
>>> http://marc.info/?l=linux-kernel&m=145552936131335&w=2
>>>
>>> I've snipped some of the quoted text ]
>>>
>>> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
>>>>
>>>> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
>>>>
>>>> * Alexander Popov <alpopov@ptsecurity.com> wrote:
>>>>
>>>>> Currently the code in fb_is_primary_device() contains to_pci_dev() macro
>>>>> which is applied to dev from struct fb_info. In some cases this causes
>>>>> bad memory access when fb_is_primary_device() handles fb_info of efifb.
>>>>> The reason is that fb dev of efifb is embedded into struct platform_device
>>>>> but not into struct pci_dev.
>>>>>
>>>>> We can fix this by checking fb dev bus name in fb_is_primary_device().
>>>>>
>>>>> It seems that this bug reveals some bigger problem with to_pci_dev(),
>>>>> to_platform_device() and others, which just do container_of() and
>>>>> don't check whether struct device is a part of the appropriate structure.
>>>>> Should we do something more about it?
>>>>>
>>>>> KASan report:
>>>
>>> [...]
>>>
>>>>>
>>>>> Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
>>>>> ---
>>>>> arch/x86/video/fbdev.c | 9 +++++----
>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
>>>>> index d5644bb..4999f78 100644
>>>>> --- a/arch/x86/video/fbdev.c
>>>>> +++ b/arch/x86/video/fbdev.c
>>>>> @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
>>>>> struct pci_dev *default_device = vga_default_device();
>>>>> struct resource *res = NULL;
>>>>>
>>>>> - if (device)
>>>>> - pci_dev = to_pci_dev(device);
>>>>> -
>>>>> - if (!pci_dev)
>>>>> + if (!device || !device->bus ||
>>>>> + !device->bus->name || strcmp(device->bus->name, "pci")) {
>>>>> return 0;
>>>>> + }
>>>>> +
>>>>> + pci_dev = to_pci_dev(device);
>>>>>
>>>>> if (default_device) {
>>>>> if (pci_dev == default_device)
>>>>> --
>>>>> 1.9.1
>>>>>
>>>
>>> I wonder if this issue could explain some of the efifb issues we've
>>> seen reported on bugzilla.kernel.org in the past where switching from
>>> efifb to some other framebuffer device caused hangs during boot. I'm
>>> struggling to find the relevant bugzilla entries now, though.
>>
>> It's possible it could, but I don't have them handy either.

[...]

>> So it's most likely right for efifb to be embedded in a platform_device
>> instead of a pci_dev. Which leads back to Alexander's question - if it
>> isn't in a pci_dev, that means fb_is_primary_device() needs to not
>> assume it is. So the patch appears correct, but so is the question -
>> should to_pci_dev() be checking this and returning NULL here?
>
> The discussion has suspended. May I activate it again?
>
> So there are two ways to fix the bad memory access in fb_is_primary_device().
>
> The first one is proposed in my patch. Checking the bus name string doesn't
> look good but I didn't manage to come up with anything better.
>
> The second way is changing to_pci_dev() similarly. It may return NULL or
> call BUG() when struct device is a part of an inappropriate structure.
>
> Which way is better? Do we need to do anything with other similar macros?

Excuse me, there is no reply for a long time. Did I touch any taboo topic?
Hope to fix this bug. Thanks.

Best regards,
Alexander



\
 
 \ /
  Last update: 2016-03-25 12:41    [W:2.290 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site