On Mon, Aug 04, 2025 at 01:00:48AM +1000, Jonathan Gray wrote:
> On Sun, Aug 03, 2025 at 02:41:55PM +0200, Hans-Jörg Höxer wrote:
> > 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?
> 
> > @@ -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;
> 
> should this and the previous case be 0xffff?

in these two cases the i/o port is specified as an 1 byte immediate.
We are reading it at uint8_t *rip.  So the "& 0xff" is actually not need,
no? I'd add it for clarity.

• smime.p7s (5K)