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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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. I've also
> wondered if some of them were due to bad data from the firmware - at
> plugfests we've seen some cases where the actual video mode as measured
> with a ruler is clearly not what the firmware claims it to be, so it's
> entirely possible we're occasionally told a memory region that is not
> what's actually mapped, or that's mapped but is only partially backed
> by the actual frame buffer memory.
>
> But aside from that diversion, I think Alexander has a legitimate
> question about use of to_pci_dev(). If I ask the question: can we fix
> this in efifb by making it live on a pci_dev, I have a couple of
> fundamental problems:
>
> 1) technically it doesn't have to be a pci_dev at all (but, practically,
> so far it always is on PCI...)
> 2) From EFI, we can't necessarily pin it down to a single PCI device
> even if it is PCI. Before we do EFI's ExitBootServices() call, we
> can try to find the PCI_IO handle our GOP instance is connected to,
> but not all firmware GOP drivers use that, so it doesn't always work.
> And even if it did, there can be more than one instance pointing to
> the same memory with different PCI devices - lots of laptops have
> this sort of thing.
> 3) Ignoring the EFI side and just focusing on PCI, if there's two
> devices configured that could do scanout, it can be mapped to one
> device's BAR but the other device be the actual device using it. In
> this case either choice is probably wrong for something, and the
> things that have the information to resolve which one don't include
> efifb - they're the drivers we'll likely hand off to later.
>
> 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?
Thanks.
Best regards,
Alexander
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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
On Fri, 25 Mar, at 02:29:21PM, Alexander Popov wrote:
>
> Excuse me, there is no reply for a long time. Did I touch any taboo topic?
> Hope to fix this bug. Thanks.
Unless anyone complains soonish I'll send this to tip as part of the
EFI urgent queue.
On Tue, 29 Mar, at 12:53:47PM, Matt Fleming wrote:
> On Fri, 25 Mar, at 02:29:21PM, Alexander Popov wrote:
> >
> > Excuse me, there is no reply for a long time. Did I touch any taboo topic?
> > Hope to fix this bug. Thanks.
>
> Unless anyone complains soonish I'll send this to tip as part of the
> EFI urgent queue.
Oh, looks like this issue was already fixed here,
https://lkml.kernel.org/r/[email protected]
* Alexander Popov <[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]>
> >>>>> ---
> >>>>> 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.
No need to worry, it's all upstream already.
Thanks,
Ingo