Re: [PATCH 2/2] s390/sclp: Implement resize for sclp-vt220 console

[Maximilian Immanuel Brandtner <maxbr@linux.ibm.com>]

On Mon, 2026-09-21 at 12:28 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [High] tty: UAPI breakage due to asynchronous winsize
> initialization in open()
> - [High] s390/sclp: out-of-bounds read in SCLP_VT220_SESSION_RESIZE
> parsing
> - [Medium] s390/sclp: data race when reading shared terminal
> dimensions
> --
> 
> commit 13e18fecbfc8440743cc8dff7c08364db9a637bf
> Author: Maximilian Immanuel Brandtner <maxbr@linux.ibm.com>
> 
> s390/sclp: Implement resize for sclp-vt220 console
> 
> This patch adds support for sclp vt220 resize events to enable host-
> initiated
> terminal resizing when using the sclp-vt220 console. This allows QEMU
> to
> dynamically resize the console.
> 
> > diff --git a/drivers/s390/char/sclp_vt220.c
> > b/drivers/s390/char/sclp_vt220.c
> > index 7c168a66e4afe..d140617846a2d 100644
> > --- a/drivers/s390/char/sclp_vt220.c
> > +++ b/drivers/s390/char/sclp_vt220.c
> 
> [ ... ]
> 
> > @@ -543,6 +588,9 @@ static void sclp_vt220_receiver_fn(struct
> > evbuf_header *evbuf)
> >  	case SCLP_VT220_SESSION_DATA:
> >  		/* Send input to line discipline */
> >  		sclp_vt220_handle_input(buffer->data, count);
> >  		tty_flip_buffer_push(&sclp_vt220_port);
> >  		break;
> > +	case SCLP_VT220_SESSION_RESIZE:
> > +		sclp_vt220_resize_sched(buffer->data);
> 
> [Severity: High]
> If the hypervisor sends an SCLP_VT220_SESSION_RESIZE event with an
> evbuf->length smaller than expected, is there a risk of reading out
> of bounds
> when the data is accessed?
> 
> This path delegates to sclp_vt220_resize_sched() without verifying if
> evbuf->length is large enough to contain the 4-byte resize payload.
> Since the
> event length is controlled by the hypervisor, could this result in an
> out-of-bounds memory read when sclp_vt220_resize_sched() reads the
> rows and
> cols fields?

Yes, but a guest needs to trust the hypervisor or firmware anyways to
provide it with correct data, so I don't see the point in verifying
data from a trusted source. (Eg what if the hypervisor implemented
load/store instructions incorrectly -- at that point there is nothing
the guest can do to operate correctly.) I could add a check here, but I
don't see the point in it.

> 
> > +		break;
> >  	}
> >  }
> >  
> > @@ -554,9 +602,9 @@ sclp_vt220_open(struct tty_struct *tty, struct
> > file *filp)
> >  {
> >  	if (tty->count == 1) {
> >  		tty_port_tty_set(&sclp_vt220_port, tty);
> > -		if (!tty->winsize.ws_row && !tty->winsize.ws_col)
> > {
> > -			tty->winsize.ws_row = 24;
> > -			tty->winsize.ws_col = 80;
> > +		if (tty->winsize.ws_row !=
> > sclp_vt220_winsize.ws_row ||
> > +		    tty->winsize.ws_col !=
> > sclp_vt220_winsize.ws_col) {
> 
> [Severity: Medium]
> Is it safe to read sclp_vt220_winsize here without synchronization?
> 
> The global sclp_vt220_winsize state is updated under sclp_vt220_lock
> in
> sclp_vt220_resize_sched(). A concurrent resize interrupt could
> theoretically
> result in a torn read here, mixing old and new terminal dimensions.

There can only be one open sclp console at a time. When opening an sclp
console there shouldn't already be stuff going on with the tty that
could mess with this. Even if this were the case though the tty is
locked by a mutex so it can't be locked in the interrupt context.
Furthermore, this behavior was already present prior to this commit so
even if it were an issue, which I don't believe it to be, it wouldn't
be an issue of this patch-set.

> 
> > +			schedule_work(&sclp_vt220_resize_work);
> 
> [Severity: High]
> Does this replace a safe synchronous initialization with an
> asynchronous one,
> exposing uninitialized dimensions to userspace?
> 
> Because workqueues execute asynchronously, the open() syscall can
> return to
> userspace before the worker thread executes tty_do_resize() to
> populate
> tty->winsize. If an application immediately queries the terminal size
> with
> ioctl(TIOCGWINSZ), wouldn't it read 0x0 instead of the actual
> dimensions?

True, but I don't consider that to be that much of a risk. There are
situations where a 0x0 terminal size can occur (though unless done
explicitly it's usually considered bad behavior) so applications need
to deal with that anyways. Most applications just fall back to 24x80
(the default). You can't assume that sclp_vt220_winsize is necessarily
24x80 once you open the console for a second time. That said in the
case of opening sclp_vt220_open().

> 
> >  		}
> >  	}
> >  	return 0;

Also you can't have your pie and eat it too. In suggestion 2 you doubt
whether you can change the window size synchronously and in suggestion
3 you mention that if you change the window size asynchronously and the
tty / application running in the tty are up really quickly and query
the winsize quickly enough they might receive an outdated result. Pick
your poison. You can't have both.

What I could see as a possible change would be removing the equality
check on tty->winsize altogether and scheduling the work
unconditionally, elliminating suggestion 2, but accepting the transient
false winsize value (which I doubt btw to ever be relevant in real
use).