| Message ID | 20230828013736.18414-3-jniethe5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] virtio-serial: Fix invalid stack access with closed virtio device | expand |
On 28/08/2023 03.37, Jordan Niethe wrote: > Commit 76fee95 ("slof: Only close stdout for virtio-serial devices") > says that commit cf28264 ("virtio-serial: Rework shutdown sequence") > fixed a hang. The problem was believed to be that it was necessary to > close stdout to shutdown the underlying virtio device. > > Commit cf28264 ("virtio-serial: Rework shutdown sequence") closed stdout > on quiesce. This meant when prom_init() called write on stdout after > quiesce, there is a use after free so this is unreliable, and can also > hang (especially after reboots). > > Quiescing is intended to put hardware into a safe state for the client > to take over. It is incorrect for SLOF to close ihandles that the client > could still be using, even after a quiesce. > > Rather than closing the stdout device, all that needs to happen is to > ensure virtio-serial-shutdown gets called. On quiesce, close the virtio > device, but leave the stdout device itself open. > > Commit 8174acd ("virtio-serial: Close device completely") handles reads > and writes as no-ops if the underlying virtio device is closed so there > is no problem with the client calling "write" on stdout after this, but > no output will be displayed. > > Fixes: cf28264 ("virtio-serial: Rework shutdown sequence") > Debugged-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > board-qemu/slof/virtio-serial.fs | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > index 41e2e04..54dacff 100644 > --- a/board-qemu/slof/virtio-serial.fs > +++ b/board-qemu/slof/virtio-serial.fs > @@ -33,8 +33,6 @@ virtio-setup-vd VALUE virtiodev > : virtio-serial-term-key? virtiodev virtio-serial-haschar ; > : virtio-serial-term-key BEGIN virtio-serial-term-key? UNTIL virtiodev virtio-serial-getchar ; > > -: virtio-serial-close-stdout s" stdout" get-chosen IF decode-int nip nip close-dev THEN ; > - > \ Basic device initialization - which has only to be done once > : init ( -- ) > virtiodev virtio-serial-init drop > @@ -42,7 +40,7 @@ virtiodev virtio-serial-init drop > \ Linux closes stdin at some point in prom_init(). This internally triggers a > \ quiesce in SLOF. We must ensure stdout gets closed as well otherwise the Please adjust the comment (since stdout now does not get closed anymore). Thanks, Thomas > \ device cannot be reset properly and the boot will hang. > - ['] virtio-serial-close-stdout add-quiesce-xt > + ['] shutdown add-quiesce-xt > ; > > 0 VALUE open-count > @@ -62,8 +60,8 @@ virtiodev virtio-serial-init drop > open-count 0> IF > open-count 1 - dup to open-count > 0= IF shutdown THEN > + close > THEN > - close > ; > > : write ( addr len -- actual )
On Mon, Aug 28, 2023 at 5:14 PM Thomas Huth <thuth@redhat.com> wrote: > > On 28/08/2023 03.37, Jordan Niethe wrote: > > Commit 76fee95 ("slof: Only close stdout for virtio-serial devices") > > says that commit cf28264 ("virtio-serial: Rework shutdown sequence") > > fixed a hang. The problem was believed to be that it was necessary to > > close stdout to shutdown the underlying virtio device. > > > > Commit cf28264 ("virtio-serial: Rework shutdown sequence") closed stdout > > on quiesce. This meant when prom_init() called write on stdout after > > quiesce, there is a use after free so this is unreliable, and can also > > hang (especially after reboots). > > > > Quiescing is intended to put hardware into a safe state for the client > > to take over. It is incorrect for SLOF to close ihandles that the client > > could still be using, even after a quiesce. > > > > Rather than closing the stdout device, all that needs to happen is to > > ensure virtio-serial-shutdown gets called. On quiesce, close the virtio > > device, but leave the stdout device itself open. > > > > Commit 8174acd ("virtio-serial: Close device completely") handles reads > > and writes as no-ops if the underlying virtio device is closed so there > > is no problem with the client calling "write" on stdout after this, but > > no output will be displayed. > > > > Fixes: cf28264 ("virtio-serial: Rework shutdown sequence") > > Debugged-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > board-qemu/slof/virtio-serial.fs | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > > index 41e2e04..54dacff 100644 > > --- a/board-qemu/slof/virtio-serial.fs > > +++ b/board-qemu/slof/virtio-serial.fs > > @@ -33,8 +33,6 @@ virtio-setup-vd VALUE virtiodev > > : virtio-serial-term-key? virtiodev virtio-serial-haschar ; > > : virtio-serial-term-key BEGIN virtio-serial-term-key? UNTIL virtiodev virtio-serial-getchar ; > > > > -: virtio-serial-close-stdout s" stdout" get-chosen IF decode-int nip nip close-dev THEN ; > > - > > \ Basic device initialization - which has only to be done once > > : init ( -- ) > > virtiodev virtio-serial-init drop > > @@ -42,7 +40,7 @@ virtiodev virtio-serial-init drop > > \ Linux closes stdin at some point in prom_init(). This internally triggers a > > \ quiesce in SLOF. We must ensure stdout gets closed as well otherwise the > > Please adjust the comment (since stdout now does not get closed anymore). Sure, will send a v2. > > Thanks, > Thomas > > > > \ device cannot be reset properly and the boot will hang. > > - ['] virtio-serial-close-stdout add-quiesce-xt > > + ['] shutdown add-quiesce-xt > > ; > > > > 0 VALUE open-count > > @@ -62,8 +60,8 @@ virtiodev virtio-serial-init drop > > open-count 0> IF > > open-count 1 - dup to open-count > > 0= IF shutdown THEN > > + close > > THEN > > - close > > ; > > > > : write ( addr len -- actual ) >
diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs index 41e2e04..54dacff 100644 --- a/board-qemu/slof/virtio-serial.fs +++ b/board-qemu/slof/virtio-serial.fs @@ -33,8 +33,6 @@ virtio-setup-vd VALUE virtiodev : virtio-serial-term-key? virtiodev virtio-serial-haschar ; : virtio-serial-term-key BEGIN virtio-serial-term-key? UNTIL virtiodev virtio-serial-getchar ; -: virtio-serial-close-stdout s" stdout" get-chosen IF decode-int nip nip close-dev THEN ; - \ Basic device initialization - which has only to be done once : init ( -- ) virtiodev virtio-serial-init drop @@ -42,7 +40,7 @@ virtiodev virtio-serial-init drop \ Linux closes stdin at some point in prom_init(). This internally triggers a \ quiesce in SLOF. We must ensure stdout gets closed as well otherwise the \ device cannot be reset properly and the boot will hang. - ['] virtio-serial-close-stdout add-quiesce-xt + ['] shutdown add-quiesce-xt ; 0 VALUE open-count @@ -62,8 +60,8 @@ virtiodev virtio-serial-init drop open-count 0> IF open-count 1 - dup to open-count 0= IF shutdown THEN + close THEN - close ; : write ( addr len -- actual )
Commit 76fee95 ("slof: Only close stdout for virtio-serial devices") says that commit cf28264 ("virtio-serial: Rework shutdown sequence") fixed a hang. The problem was believed to be that it was necessary to close stdout to shutdown the underlying virtio device. Commit cf28264 ("virtio-serial: Rework shutdown sequence") closed stdout on quiesce. This meant when prom_init() called write on stdout after quiesce, there is a use after free so this is unreliable, and can also hang (especially after reboots). Quiescing is intended to put hardware into a safe state for the client to take over. It is incorrect for SLOF to close ihandles that the client could still be using, even after a quiesce. Rather than closing the stdout device, all that needs to happen is to ensure virtio-serial-shutdown gets called. On quiesce, close the virtio device, but leave the stdout device itself open. Commit 8174acd ("virtio-serial: Close device completely") handles reads and writes as no-ops if the underlying virtio device is closed so there is no problem with the client calling "write" on stdout after this, but no output will be displayed. Fixes: cf28264 ("virtio-serial: Rework shutdown sequence") Debugged-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- board-qemu/slof/virtio-serial.fs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)