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;

bluhm

> > -----------------------------------------------------------------------
> > commit 0bdf00d115852dabeb9078497d91e19d8667898f
> > 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..dcdfd98c909 100644
> > --- a/sys/arch/amd64/amd64/trap.c
> > +++ b/sys/arch/amd64/amd64/trap.c
> > @@ -367,14 +367,14 @@ vctrap(struct trapframe *frame, int user)
> >  			case 0xef:	/* out %ax,(%dx) */
> >  				ghcb_sync_val(GHCB_RAX, GHCB_SZ16, &syncout);
> >  				port = (uint16_t)frame->tf_rdx;
> > -				sw_exitinfo1 = (port << 16) |
> > +				sw_exitinfo1 = ((uint64_t)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;
> > -				sw_exitinfo1 = (port << 16) |
> > +				sw_exitinfo1 = ((uint64_t)port << 16) |
> >  				    (1ULL << 5) | (1ULL << 0);
> >  				frame->tf_rip += 2;
> >  				break;
> > @@ -386,40 +386,40 @@ vctrap(struct trapframe *frame, int user)
> >  		case 0xe4:	/* in $port,%al */
> >  			ghcb_sync_val(GHCB_RAX, GHCB_SZ8, &syncin);
> >  			port = *(rip + 1);
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 4) |
> > +			sw_exitinfo1 = ((uint64_t)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);
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 4);
> > +			sw_exitinfo1 = ((uint64_t)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;
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 4) |
> > +			sw_exitinfo1 = ((uint64_t)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;
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 6) |
> > +			sw_exitinfo1 = ((uint64_t)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;
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 4);
> > +			sw_exitinfo1 = ((uint64_t)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;
> > -			sw_exitinfo1 = (port << 16) | (1ULL << 6);
> > +			sw_exitinfo1 = ((uint64_t)port << 16) | (1ULL << 6);
> >  			frame->tf_rip += 1;
> >  			break;
> >  		default:
> 
> 
>