From: Hans-Jörg Höxer <hshoexer@genua.de>
Subject: Re: [EXT] Re: Avoid implicit sign extension in vctrap()
To: <tech@openbsd.org>
Date: Sun, 3 Aug 2025 14:41:55 +0200

Hi,

On Sun, Aug 03, 2025 at 10:57:28AM +1000, Jonathan Gray wrote:
> On Sun, Aug 03, 2025 at 02:34:20AM +0200, Alexander Bluhm wrote:
> > On Sun, Aug 03, 2025 at 09:37:21AM +1000, Jonathan Gray wrote:
> > > On Sat, Aug 02, 2025 at 11:24:04PM +0200, Hans-J?rg H?xer wrote:
> > > > Hi,
> > > > 
> > > > found by coverty:
> > > >     
> > > >     port is uint16_t.  Shifting promotes to signed 32bit integer.
> > > >     When assigning to uint64_t sw_exitinfo1 the value gets sign extended.
> > > >     Thus when "port << 16" is greater than 0x7fffffff the upper bits
> > > >     of sw_exitinfo1 will be all 1.  Therefore cast to uint64_t before
> > > >     shifting.
> > > > 
> > > > ok?
> > > 
> > > or make port uint64_t and drop the casts?
> > 
> > We must truncate the value here.
> > 
> > port = (uint16_t)frame->tf_rdx;
> > 
> > In theory the (uint16_t) cast should do that.  But I feel safer
> > with a 16 bit variable for that.
> > 
> > Or we could use an idiom without any casts.
> > 
> > uint64_t port = frame->tf_rdx & 0xffff;
> 
> I find that more readable.  I was looking for a later use of port that
> required it to be 16-bit.  Wasn't clear it was masked with a cast.
> 
> The 1ULL parts could just be 1.

updated diff below.

ok?

hshoexer
-----------------------------------------------------------------------------
commit c9c614dac43c7a5d2318de7b143dcb6b4067658e
Author: Hans-Joerg Hoexer <hshoexer@genua.de>
Date:   Sat Aug 2 23:04:22 2025 +0200

    Avoid implicit sign extension in vctrap()
    
    port is uint16_t.  Shifting promotes to signed 32bit integer.  When
    assigning to uint64_t sw_exitinfo1 the value gets sign extended.
    Thus when "port << 16" is greater than 0x7fffffff the upper bits
    of sw_exitinfo1 will be all 1.  Therefore apply cast to uint64_t
    before shifting.
    
    Coverity CID 1648416.

diff --git a/sys/arch/amd64/amd64/trap.c b/sys/arch/amd64/amd64/trap.c
index cb152600dca..4c32504edb9 100644
--- a/sys/arch/amd64/amd64/trap.c
+++ b/sys/arch/amd64/amd64/trap.c
@@ -306,7 +306,7 @@ vctrap(struct trapframe *frame, int user)
 {
 	uint64_t	 sw_exitcode, sw_exitinfo1, sw_exitinfo2;
 	uint8_t		*rip = (uint8_t *)(frame->tf_rip);
-	uint16_t	 port;
+	uint64_t	 port;
 	struct ghcb_sync syncout, syncin;
 	struct ghcb_sa	*ghcb;
 
@@ -366,14 +366,14 @@ vctrap(struct trapframe *frame, int user)
 			switch (*(rip + 1)) {
 			case 0xef:	/* out %ax,(%dx) */
 				ghcb_sync_val(GHCB_RAX, GHCB_SZ16, &syncout);
-				port = (uint16_t)frame->tf_rdx;
+				port = frame->tf_rdx & 0xffff;
 				sw_exitinfo1 = (port << 16) |
 				    (1ULL << 5);
 				frame->tf_rip += 2;
 				break;
 			case 0xed:	/* in (%dx),%ax */
 				ghcb_sync_val(GHCB_RAX, GHCB_SZ16, &syncin);
-				port = (uint16_t)frame->tf_rdx;
+				port = frame->tf_rdx & 0xffff;
 				sw_exitinfo1 = (port << 16) |
 				    (1ULL << 5) | (1ULL << 0);
 				frame->tf_rip += 2;
@@ -385,40 +385,40 @@ vctrap(struct trapframe *frame, int user)
 		    }
 		case 0xe4:	/* in $port,%al */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ8, &syncin);
-			port = *(rip + 1);
+			port = *(rip + 1) & 0xff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 4) |
 			    (1ULL << 0);
 			frame->tf_rip += 2;
 			break;
 		case 0xe6:	/* outb %al,$port */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ8, &syncout);
-			port = *(rip + 1);
+			port = *(rip + 1) & 0xff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 4);
 			frame->tf_rip += 2;
 			break;
 		case 0xec:	/* in (%dx),%al */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ8, &syncin);
-			port = (uint16_t)frame->tf_rdx;
+			port = frame->tf_rdx & 0xffff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 4) |
 			    (1ULL << 0);
 			frame->tf_rip += 1;
 			break;
 		case 0xed:	/* in (%dx),%eax */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ32, &syncin);
-			port = (uint16_t)frame->tf_rdx;
+			port = frame->tf_rdx & 0xffff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 6) |
 			    (1ULL << 0);
 			frame->tf_rip += 1;
 			break;
 		case 0xee:	/* out %al,(%dx) */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ8, &syncout);
-			port = (uint16_t)frame->tf_rdx;
+			port = frame->tf_rdx & 0xffff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 4);
 			frame->tf_rip += 1;
 			break;
 		case 0xef:	/* out %eax,(%dx) */
 			ghcb_sync_val(GHCB_RAX, GHCB_SZ32, &syncout);
-			port = (uint16_t)frame->tf_rdx;
+			port = frame->tf_rdx & 0xffff;
 			sw_exitinfo1 = (port << 16) | (1ULL << 6);
 			frame->tf_rip += 1;
 			break;