fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

[Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Mark Cave-Ayland 5 years, 5 months ago

Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
non-DMA transfers.

If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
reference isn't initialised during isabus_fdc_realize(). Unfortunately
fdctrl_stop_transfer() unconditionally references the DMA interface when
finishing the transfer causing a NULL pointer dereference.

Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
interface reference and release method is only invoked if fdctrl->dma_chann
has been set.

(This issue was discovered by Martin testing a recent change in the NetBSD
installer under qemu-system-sparc)

Reported-by: Martin Husemann <martin@duskware.de>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/block/fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2e9c1e1e2f..6f19f127a5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     fdctrl->fifo[5] = cur_drv->sect;
     fdctrl->fifo[6] = FD_SECTOR_SC;
     fdctrl->data_dir = FD_DIR_READ;
-    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
+    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
         IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
         k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
     }
-- 
2.11.0

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On Sun, Nov 11, 2018 at 10:41 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
> non-DMA transfers.
>
> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
> reference isn't initialised during isabus_fdc_realize(). Unfortunately
> fdctrl_stop_transfer() unconditionally references the DMA interface when
> finishing the transfer causing a NULL pointer dereference.
>
> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
> interface reference and release method is only invoked if fdctrl->dma_chann
> has been set.
>
> (This issue was discovered by Martin testing a recent change in the NetBSD
> installer under qemu-system-sparc)
>
> Reported-by: Martin Husemann <martin@duskware.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/block/fdc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2e9c1e1e2f..6f19f127a5 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>      fdctrl->fifo[5] = cur_drv->sect;
>      fdctrl->fifo[6] = FD_SECTOR_SC;
>      fdctrl->data_dir = FD_DIR_READ;
> -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
> +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
>          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
>      }
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Hervé Poussineau 5 years, 5 months ago

Le 11/11/2018 à 10:40, Mark Cave-Ayland a écrit :
> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
> non-DMA transfers.
> 
> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
> reference isn't initialised during isabus_fdc_realize(). Unfortunately
> fdctrl_stop_transfer() unconditionally references the DMA interface when
> finishing the transfer causing a NULL pointer dereference.
> 
> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
> interface reference and release method is only invoked if fdctrl->dma_chann
> has been set.
> 
> (This issue was discovered by Martin testing a recent change in the NetBSD
> installer under qemu-system-sparc)
> 
> Reported-by: Martin Husemann <martin@duskware.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by John Snow 5 years, 5 months ago


On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
> non-DMA transfers.
> 
> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
> reference isn't initialised during isabus_fdc_realize(). Unfortunately
> fdctrl_stop_transfer() unconditionally references the DMA interface when
> finishing the transfer causing a NULL pointer dereference.
> 
> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
> interface reference and release method is only invoked if fdctrl->dma_chann
> has been set.
> 
> (This issue was discovered by Martin testing a recent change in the NetBSD
> installer under qemu-system-sparc)
> 
> Reported-by: Martin Husemann <martin@duskware.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/block/fdc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2e9c1e1e2f..6f19f127a5 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>      fdctrl->fifo[5] = cur_drv->sect;
>      fdctrl->fifo[6] = FD_SECTOR_SC;
>      fdctrl->data_dir = FD_DIR_READ;
> -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
> +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
>          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
>          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
>      }
> 

Thanks.

Reviewed-by: John Snow <jsnow@redhat.com>

... Kevin, would you mind staging this one-off for the next RC?

--js

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Kevin Wolf 5 years, 5 months ago

Am 12.11.2018 um 20:58 hat John Snow geschrieben:
> 
> 
> On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
> > Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
> > functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
> > non-DMA transfers.
> > 
> > If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
> > reference isn't initialised during isabus_fdc_realize(). Unfortunately
> > fdctrl_stop_transfer() unconditionally references the DMA interface when
> > finishing the transfer causing a NULL pointer dereference.
> > 
> > Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
> > interface reference and release method is only invoked if fdctrl->dma_chann
> > has been set.
> > 
> > (This issue was discovered by Martin testing a recent change in the NetBSD
> > installer under qemu-system-sparc)
> > 
> > Reported-by: Martin Husemann <martin@duskware.de>
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >  hw/block/fdc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index 2e9c1e1e2f..6f19f127a5 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
> >      fdctrl->fifo[5] = cur_drv->sect;
> >      fdctrl->fifo[6] = FD_SECTOR_SC;
> >      fdctrl->data_dir = FD_DIR_READ;
> > -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
> > +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
> >          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
> >          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
> >      }
> > 
> 
> Thanks.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> ... Kevin, would you mind staging this one-off for the next RC?

No problem, I'm applying this to my block branch. However, my pull
request for -rc1 is already merged, so this will have to wait until next
week and -rc2.

Kevin

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by John Snow 5 years, 5 months ago


On 11/13/18 8:16 AM, Kevin Wolf wrote:
> Am 12.11.2018 um 20:58 hat John Snow geschrieben:
>>
>>
>> On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
>>> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
>>> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
>>> non-DMA transfers.
>>>
>>> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
>>> reference isn't initialised during isabus_fdc_realize(). Unfortunately
>>> fdctrl_stop_transfer() unconditionally references the DMA interface when
>>> finishing the transfer causing a NULL pointer dereference.
>>>
>>> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
>>> interface reference and release method is only invoked if fdctrl->dma_chann
>>> has been set.
>>>
>>> (This issue was discovered by Martin testing a recent change in the NetBSD
>>> installer under qemu-system-sparc)
>>>
>>> Reported-by: Martin Husemann <martin@duskware.de>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/block/fdc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 2e9c1e1e2f..6f19f127a5 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>>>      fdctrl->fifo[5] = cur_drv->sect;
>>>      fdctrl->fifo[6] = FD_SECTOR_SC;
>>>      fdctrl->data_dir = FD_DIR_READ;
>>> -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
>>> +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
>>>          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
>>>          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
>>>      }
>>>
>>
>> Thanks.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> ... Kevin, would you mind staging this one-off for the next RC?
> 
> No problem, I'm applying this to my block branch. However, my pull
> request for -rc1 is already merged, so this will have to wait until next
> week and -rc2.
> 
> Kevin
> 

I think that should be fine. Thank you!

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Mark Cave-Ayland 5 years, 5 months ago

On 13/11/2018 20:29, John Snow wrote:

> On 11/13/18 8:16 AM, Kevin Wolf wrote:
>> Am 12.11.2018 um 20:58 hat John Snow geschrieben:
>>>
>>>
>>> On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
>>>> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
>>>> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
>>>> non-DMA transfers.
>>>>
>>>> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
>>>> reference isn't initialised during isabus_fdc_realize(). Unfortunately
>>>> fdctrl_stop_transfer() unconditionally references the DMA interface when
>>>> finishing the transfer causing a NULL pointer dereference.
>>>>
>>>> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
>>>> interface reference and release method is only invoked if fdctrl->dma_chann
>>>> has been set.
>>>>
>>>> (This issue was discovered by Martin testing a recent change in the NetBSD
>>>> installer under qemu-system-sparc)
>>>>
>>>> Reported-by: Martin Husemann <martin@duskware.de>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  hw/block/fdc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 2e9c1e1e2f..6f19f127a5 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>>>>      fdctrl->fifo[5] = cur_drv->sect;
>>>>      fdctrl->fifo[6] = FD_SECTOR_SC;
>>>>      fdctrl->data_dir = FD_DIR_READ;
>>>> -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
>>>> +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
>>>>          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
>>>>          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
>>>>      }
>>>>
>>>
>>> Thanks.
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>> ... Kevin, would you mind staging this one-off for the next RC?
>>
>> No problem, I'm applying this to my block branch. However, my pull
>> request for -rc1 is already merged, so this will have to wait until next
>> week and -rc2.
>>
>> Kevin
>>
> 
> I think that should be fine. Thank you!

Thanks everyone! Kevin, any chance you could also add a CC: qemu-stable@ tag when you
apply this to your branch? This will help ensure that when the next NetBSD release
appears the fix should already be available for most people.


ATB,

Mark.

Re: [Qemu-devel] [PATCH for-3.1] fdc: fix segfault in fdctrl_stop_transfer() when DMA is disabled

Posted by Kevin Wolf 5 years, 5 months ago

Am 18.11.2018 um 13:32 hat Mark Cave-Ayland geschrieben:
> On 13/11/2018 20:29, John Snow wrote:
> 
> > On 11/13/18 8:16 AM, Kevin Wolf wrote:
> >> Am 12.11.2018 um 20:58 hat John Snow geschrieben:
> >>>
> >>>
> >>> On 11/11/18 4:40 AM, Mark Cave-Ayland wrote:
> >>>> Commit c8a35f1cf0f "fdc: use IsaDma interface instead of global DMA_*
> >>>> functions" accidentally introduced a segfault in fdctrl_stop_transfer() for
> >>>> non-DMA transfers.
> >>>>
> >>>> If fdctrl->dma_chann has not been configured then the fdctrl->dma interface
> >>>> reference isn't initialised during isabus_fdc_realize(). Unfortunately
> >>>> fdctrl_stop_transfer() unconditionally references the DMA interface when
> >>>> finishing the transfer causing a NULL pointer dereference.
> >>>>
> >>>> Fix the issue by adding a check in fdctrl_stop_transfer() so that the DMA
> >>>> interface reference and release method is only invoked if fdctrl->dma_chann
> >>>> has been set.
> >>>>
> >>>> (This issue was discovered by Martin testing a recent change in the NetBSD
> >>>> installer under qemu-system-sparc)
> >>>>
> >>>> Reported-by: Martin Husemann <martin@duskware.de>
> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>> ---
> >>>>  hw/block/fdc.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> >>>> index 2e9c1e1e2f..6f19f127a5 100644
> >>>> --- a/hw/block/fdc.c
> >>>> +++ b/hw/block/fdc.c
> >>>> @@ -1617,7 +1617,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
> >>>>      fdctrl->fifo[5] = cur_drv->sect;
> >>>>      fdctrl->fifo[6] = FD_SECTOR_SC;
> >>>>      fdctrl->data_dir = FD_DIR_READ;
> >>>> -    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
> >>>> +    if (fdctrl->dma_chann != -1 && !(fdctrl->msr & FD_MSR_NONDMA)) {
> >>>>          IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
> >>>>          k->release_DREQ(fdctrl->dma, fdctrl->dma_chann);
> >>>>      }
> >>>>
> >>>
> >>> Thanks.
> >>>
> >>> Reviewed-by: John Snow <jsnow@redhat.com>
> >>>
> >>> ... Kevin, would you mind staging this one-off for the next RC?
> >>
> >> No problem, I'm applying this to my block branch. However, my pull
> >> request for -rc1 is already merged, so this will have to wait until next
> >> week and -rc2.
> >>
> >> Kevin
> >>
> > 
> > I think that should be fine. Thank you!
> 
> Thanks everyone! Kevin, any chance you could also add a CC: qemu-stable@ tag when you
> apply this to your branch? This will help ensure that when the next NetBSD release
> appears the fix should already be available for most people.

Ok, done. Also actually CCed qemu-stable on this mail.

Kevin