On Mon, 07 Sep 2026 20:50:45 +0200,
"Theo de Raadt" <deraadt@openbsd.org> wrote:
> 
> Theo de Raadt <deraadt@openbsd.org> wrote:
> 
> > Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > > my update libfido accidently reverted a hardcoded USB device metadata which
> > > leads broken pledge in, for example, chromium.
> > 
> > I'm more willing to allow pledge to reach ioctl USB_GET_DEVICEINFO, if
> > there are some incremental safety improvements made to the kernel code
> > that is reached.
> > 
> > I think usbd_fill_deviceinfo() should return int, look carefully at
> > what it is going to provide to userland, and return an error if the
> > usb configuration is not sane.
> > 
> > But first, I think we should purge old data in the structure.
> > 
> > Index: usb_subr.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v
> > diff -u -p -u -r1.167 usb_subr.c
> > --- usb_subr.c	15 Aug 2026 22:06:40 -0000	1.167
> > +++ usb_subr.c	7 Sep 2026 18:36:54 -0000
> > @@ -1298,6 +1298,7 @@ usbd_fill_deviceinfo(struct usbd_device 
> >  	struct usbd_port *p;
> >  	int i;
> >  
> > +	memset(di, 0, sizeof(*di));
> >  	di->udi_bus = dev->bus->usbctl->dv_unit;
> >  	di->udi_addr = dev->address;
> >  	strlcpy(di->udi_vendor, dev->vendor, sizeof(di->udi_vendor));
> 
> If the structure is zero'd, then usbd_fill_deviceinfo() only needs
> to focus on filling in non-zero values, which looks like this:
>

Which reads quite sane and make code simpler. I like it.

> Index: usb_subr.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v
> diff -u -p -u -r1.167 usb_subr.c
> --- usb_subr.c	15 Aug 2026 22:06:40 -0000	1.167
> +++ usb_subr.c	7 Sep 2026 18:48:37 -0000
> @@ -1298,6 +1298,7 @@ usbd_fill_deviceinfo(struct usbd_device 
>  	struct usbd_port *p;
>  	int i;
>  
> +	memset(di, 0, sizeof(*di));
>  	di->udi_bus = dev->bus->usbctl->dv_unit;
>  	di->udi_addr = dev->address;
>  	strlcpy(di->udi_vendor, dev->vendor, sizeof(di->udi_vendor));
> @@ -1316,16 +1317,10 @@ usbd_fill_deviceinfo(struct usbd_device 
>  	di->udi_port = dev->powersrc ? dev->powersrc->portno : 0;
>  
>  	if (dev->subdevs != NULL) {
> -		for (i = 0; dev->subdevs[i] && i < USB_MAX_DEVNAMES; i++) {
> -			strncpy(di->udi_devnames[i],
> +		for (i = 0; dev->subdevs[i] && i < USB_MAX_DEVNAMES; i++)
> +			strlcpy(di->udi_devnames[i],
>  			    dev->subdevs[i]->dv_xname, USB_MAX_DEVNAMELEN);
> -			di->udi_devnames[i][USB_MAX_DEVNAMELEN-1] = '\0';
> -		}
> -	} else
> -		i = 0;
> -
> -	for (/*i is set */; i < USB_MAX_DEVNAMES; i++)
> -		di->udi_devnames[i][0] = 0; /* empty */
> +	}
>  
>  	if (dev->hub) {
>  		for (i = 0;
> @@ -1335,13 +1330,10 @@ usbd_fill_deviceinfo(struct usbd_device 
>  			    UGETW(p->status.wPortStatus);
>  		}
>  		di->udi_nports = dev->hub->nports;
> -	} else
> -		di->udi_nports = 0;
> +	}
>  
> -	bzero(di->udi_serial, sizeof(di->udi_serial));
>  	if (dev->serial != NULL)
> -		strlcpy(di->udi_serial, dev->serial,
> -		    sizeof(di->udi_serial));
> +		strlcpy(di->udi_serial, dev->serial, sizeof(di->udi_serial));
>  }
>  
>  int
> 

-- 
wbr, Kirill