[smbios] Remove userptr_t from SMBIOS structure parsing

Simplify the SMBIOS structure parsing code by assuming that all
structure content is fully accessible via pointer dereferences.

In particular, this allows the convoluted find_smbios_structure() and
read_smbios_structure() to be combined into a single function
smbios_structure() that just returns a direct pointer to the SMBIOS
structure, with smbios_string() similarly now returning a direct
pointer to the relevant string.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

src/arch/x86/interface/pcbios/bios_smbios.c

@@ -45,19 +45,18 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
  * @ret rc		Return status code
  */
 static int bios_find_smbios2 ( struct smbios *smbios ) {
-	struct smbios_entry entry;
+	const struct smbios_entry *entry;
-	int rc;
 
 	/* Scan through BIOS segment to find SMBIOS 32-bit entry point */
-	if ( ( rc = find_smbios_entry ( real_to_virt ( BIOS_SEG, 0 ), 0x10000,
+	entry = find_smbios_entry ( real_to_virt ( BIOS_SEG, 0 ), 0x10000 );
-					&entry ) ) != 0 )
+	if ( ! entry )
-		return rc;
+		return -ENOENT;
 
 	/* Fill in entry point descriptor structure */
-	smbios->address = phys_to_virt ( entry.smbios_address );
+	smbios->address = phys_to_virt ( entry->smbios_address );
-	smbios->len = entry.smbios_len;
+	smbios->len = entry->smbios_len;
-	smbios->count = entry.smbios_count;
+	smbios->count = entry->smbios_count;
-	smbios->version = SMBIOS_VERSION ( entry.major, entry.minor );
+	smbios->version = SMBIOS_VERSION ( entry->major, entry->minor );
 
 	return 0;
 }
@@ -69,26 +68,25 @@ static int bios_find_smbios2 ( struct smbios *smbios ) {
  * @ret rc		Return status code
  */
 static int bios_find_smbios3 ( struct smbios *smbios ) {
-	struct smbios3_entry entry;
+	const struct smbios3_entry *entry;
-	int rc;
 
 	/* Scan through BIOS segment to find SMBIOS 64-bit entry point */
-	if ( ( rc = find_smbios3_entry ( real_to_virt ( BIOS_SEG, 0 ), 0x10000,
+	entry = find_smbios3_entry ( real_to_virt ( BIOS_SEG, 0 ), 0x10000 );
-					 &entry ) ) != 0 )
+	if ( ! entry )
-		return rc;
+		return -ENOENT;
 
 	/* Check that address is accessible */
-	if ( entry.smbios_address > ~( ( physaddr_t ) 0 ) ) {
+	if ( entry->smbios_address > ~( ( physaddr_t ) 0 ) ) {
 		DBG ( "SMBIOS3 at %08llx is inaccessible\n",
-		      ( ( unsigned long long ) entry.smbios_address ) );
+		      ( ( unsigned long long ) entry->smbios_address ) );
 		return -ENOTSUP;
 	}
 
 	/* Fill in entry point descriptor structure */
-	smbios->address = phys_to_virt ( entry.smbios_address );
+	smbios->address = phys_to_virt ( entry->smbios_address );
-	smbios->len = entry.smbios_len;
+	smbios->len = entry->smbios_len;
 	smbios->count = 0;
-	smbios->version = SMBIOS_VERSION ( entry.major, entry.minor );
+	smbios->version = SMBIOS_VERSION ( entry->major, entry->minor );
 
 	return 0;
 }

src/drivers/net/smsc95xx.c

@@ -64,92 +64,67 @@ static struct profiler smsc95xx_out_profiler __profiler =
  */
 static int smsc95xx_vm3_fetch_mac ( struct smscusb_device *smscusb ) {
 	struct net_device *netdev = smscusb->netdev;
-	struct smbios_structure structure;
+	const struct smbios_header *structure;
-	struct smbios_system_information system;
+	const struct smbios_system_information *system;
-	struct {
+	const char *manufacturer;
-		char manufacturer[ 10 /* "Honeywell" + NUL */ ];
+	const char *product;
-		char product[ 4 /* "VM3" + NUL */ ];
+	const char *mac;
-		char mac[ base16_encoded_len ( ETH_ALEN ) + 1 /* NUL */ ];
-	} strings;
 	int len;
 	int rc;
 
 	/* Find system information */
-	if ( ( rc = find_smbios_structure ( SMBIOS_TYPE_SYSTEM_INFORMATION, 0,
+	structure = smbios_structure ( SMBIOS_TYPE_SYSTEM_INFORMATION, 0 );
-					    &structure ) ) != 0 ) {
+	if ( ! structure ) {
 		DBGC ( smscusb, "SMSC95XX %p could not find system "
-		       "information: %s\n", smscusb, strerror ( rc ) );
+		       "information\n", smscusb );
-		return rc;
+		return -ENOENT;
 	}
-
+	system = container_of ( structure, struct smbios_system_information,
-	/* Read system information */
+				header );
-	if ( ( rc = read_smbios_structure ( &structure, &system,
-					    sizeof ( system ) ) ) != 0 ) {
-		DBGC ( smscusb, "SMSC95XX %p could not read system "
-		       "information: %s\n", smscusb, strerror ( rc ) );
-		return rc;
-	}
-
-	/* NUL-terminate all strings to be fetched */
-	memset ( &strings, 0, sizeof ( strings ) );
 
 	/* Fetch system manufacturer name */
-	len = read_smbios_string ( &structure, system.manufacturer,
+	manufacturer = smbios_string ( structure, system->manufacturer );
-				   strings.manufacturer,
+	if ( ! manufacturer ) {
-				   ( sizeof ( strings.manufacturer ) - 1 ) );
-	if ( len < 0 ) {
-		rc = len;
 		DBGC ( smscusb, "SMSC95XX %p could not read manufacturer "
-		       "name: %s\n", smscusb, strerror ( rc ) );
+		       "name\n", smscusb );
-		return rc;
+		return -ENOENT;
 	}
 
 	/* Fetch system product name */
-	len = read_smbios_string ( &structure, system.product, strings.product,
+	product = smbios_string ( structure, system->product );
-				   ( sizeof ( strings.product ) - 1 ) );
+	if ( ! product ) {
-	if ( len < 0 ) {
+		DBGC ( smscusb, "SMSC95XX %p could not read product name\n",
-		rc = len;
+		       smscusb );
-		DBGC ( smscusb, "SMSC95XX %p could not read product name: "
+		return -ENOENT;
-		       "%s\n", smscusb, strerror ( rc ) );
-		return rc;
 	}
 
 	/* Ignore non-VM3 devices */
-	if ( ( strcmp ( strings.manufacturer, "Honeywell" ) != 0 ) ||
+	if ( ( strcmp ( manufacturer, "Honeywell" ) != 0 ) ||
-	     ( strcmp ( strings.product, "VM3" ) != 0 ) )
+	     ( strcmp ( product, "VM3" ) != 0 ) )
 		return -ENOTTY;
 
 	/* Find OEM strings */
-	if ( ( rc = find_smbios_structure ( SMBIOS_TYPE_OEM_STRINGS, 0,
+	structure = smbios_structure ( SMBIOS_TYPE_OEM_STRINGS, 0 );
-					    &structure ) ) != 0 ) {
+	if ( ! structure ) {
-		DBGC ( smscusb, "SMSC95XX %p could not find OEM strings: %s\n",
+		DBGC ( smscusb, "SMSC95XX %p could not find OEM strings\n",
-		       smscusb, strerror ( rc ) );
+		       smscusb );
-		return rc;
+		return -ENOENT;
 	}
 
 	/* Fetch MAC address */
-	len = read_smbios_string ( &structure, SMSC95XX_VM3_OEM_STRING_MAC,
+	mac = smbios_string ( structure, SMSC95XX_VM3_OEM_STRING_MAC );
-				   strings.mac, ( sizeof ( strings.mac ) - 1 ));
+	if ( ! mac ) {
-	if ( len < 0 ) {
+		DBGC ( smscusb, "SMSC95XX %p could not read OEM string\n",
-		rc = len;
+		       smscusb );
-		DBGC ( smscusb, "SMSC95XX %p could not read OEM string: %s\n",
+		return -ENOENT;
-		       smscusb, strerror ( rc ) );
-		return rc;
-	}
-
-	/* Sanity check */
-	if ( len != ( ( int ) ( sizeof ( strings.mac ) - 1 ) ) ) {
-		DBGC ( smscusb, "SMSC95XX %p invalid MAC address \"%s\"\n",
-		       smscusb, strings.mac );
-		return -EINVAL;
 	}
 
 	/* Decode MAC address */
-	len = base16_decode ( strings.mac, netdev->hw_addr, ETH_ALEN );
+	len = base16_decode ( mac, netdev->hw_addr, ETH_ALEN );
 	if ( len < 0 ) {
 		rc = len;
 		DBGC ( smscusb, "SMSC95XX %p invalid MAC address \"%s\"\n",
-		       smscusb, strings.mac );
+		       smscusb, mac );
 		return rc;
 	}
 

src/include/ipxe/smbios.h

@@ -12,7 +12,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 #include <stdint.h>
 #include <ipxe/api.h>
 #include <config/general.h>
-#include <ipxe/uaccess.h>
 
 /**
  * Provide an SMBIOS API implementation
@@ -126,16 +125,6 @@ struct smbios_header {
 	uint16_t handle;
 } __attribute__ (( packed ));
 
-/** SMBIOS structure descriptor */
-struct smbios_structure {
-	/** Copy of SMBIOS structure header */
-	struct smbios_header header;
-	/** Offset of structure within SMBIOS */
-	size_t offset;
-	/** Length of strings section */
-	size_t strings_len;
-};
-
 /** SMBIOS system information structure */
 struct smbios_system_information {
 	/** SMBIOS structure header */
@@ -207,7 +196,7 @@ struct smbios_enclosure_information {
  */
 struct smbios {
 	/** Start of SMBIOS structures */
-	userptr_t address;
+	const void *address;
 	/** Length of SMBIOS structures */
 	size_t len;
 	/** Number of SMBIOS structures */
@@ -226,17 +215,14 @@ struct smbios {
 #define SMBIOS_VERSION( major, minor ) ( ( (major) << 8 ) | (minor) )
 
 extern int find_smbios ( struct smbios *smbios );
-extern int find_smbios_entry ( userptr_t start, size_t len,
+extern const struct smbios_entry * find_smbios_entry ( const void *start,
-			       struct smbios_entry *entry );
+						       size_t len );
-extern int find_smbios3_entry ( userptr_t start, size_t len,
+extern const struct smbios3_entry * find_smbios3_entry ( const void *start,
-				struct smbios3_entry *entry );
+							 size_t len );
-extern int find_smbios_structure ( unsigned int type, unsigned int instance,
+extern const struct smbios_header * smbios_structure ( unsigned int type,
-				   struct smbios_structure *structure );
+						       unsigned int instance );
-extern int read_smbios_structure ( struct smbios_structure *structure,
+extern const char * smbios_string ( const struct smbios_header *header,
-				   void *data, size_t len );
+				    unsigned int index );
-extern int read_smbios_string ( struct smbios_structure *structure,
-				unsigned int index,
-				void *data, size_t len );
 extern int smbios_version ( void );
 extern void smbios_clear ( void );
 

src/interface/efi/efi_smbios.c

@@ -20,6 +20,7 @@
 FILE_LICENCE ( GPL2_OR_LATER );
 
 #include <errno.h>
+#include <ipxe/uaccess.h>
 #include <ipxe/smbios.h>
 #include <ipxe/efi/efi.h>
 #include <ipxe/efi/Guid/SmBios.h>

src/interface/linux/linux_smbios.c

@@ -35,7 +35,7 @@ static const char smbios_entry_filename[] =
 static const char smbios_filename[] = "/sys/firmware/dmi/tables/DMI";
 
 /** Cache SMBIOS data */
-static userptr_t smbios_data;
+static void *smbios_data;
 
 /**
  * Find SMBIOS
@@ -46,7 +46,7 @@ static userptr_t smbios_data;
 static int linux_find_smbios ( struct smbios *smbios ) {
 	struct smbios3_entry *smbios3_entry;
 	struct smbios_entry *smbios_entry;
-	userptr_t entry;
+	void *entry;
 	void *data;
 	int len;
 	int rc;
@@ -98,6 +98,7 @@ static int linux_find_smbios ( struct smbios *smbios ) {
 	return 0;
 
 	ufree ( smbios_data );
+	smbios_data = NULL;
  err_read:
  err_version:
 	ufree ( entry );
@@ -116,6 +117,7 @@ static void linux_smbios_shutdown ( int booting __unused ) {
 
 	/* Free SMBIOS data */
 	ufree ( smbios_data );
+	smbios_data = NULL;
 }
 
 /** SMBIOS shutdown function */

src/interface/smbios/smbios.c

@@ -38,26 +38,24 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 
 /** SMBIOS entry point descriptor */
 static struct smbios smbios = {
-	.address = UNULL,
+	.address = NULL,
 };
 
 /**
  * Calculate SMBIOS entry point structure checksum
  *
  * @v start		Start address of region
- * @v offset		Offset of SMBIOS entry point structure
  * @v len		Length of entry point structure
  * @ret sum		Byte checksum
  */
-static uint8_t smbios_checksum ( userptr_t start, size_t offset, size_t len ) {
+static uint8_t smbios_checksum ( const void *start, size_t len ) {
-	size_t end = ( offset + len );
+	const uint8_t *byte = start;
-	uint8_t sum;
+	uint8_t sum = 0;
-	uint8_t byte;
+
+	/* Compute checksum */
+	while ( len-- )
+		sum += *(byte++);
 
-	for ( sum = 0 ; offset < end ; offset++ ) {
-		copy_from_user ( &byte, start, offset, sizeof ( byte ) );
-		sum += byte;
-	}
 	return sum;
 }
 
@@ -66,39 +64,45 @@ static uint8_t smbios_checksum ( userptr_t start, size_t offset, size_t len ) {
  *
  * @v start		Start address of region to scan
  * @v len		Length of region to scan
- * @v entry		SMBIOS entry point structure to fill in
+ * @ret entry		SMBIOS entry point structure, or NULL if not found
- * @ret rc		Return status code
  */
-int find_smbios_entry ( userptr_t start, size_t len,
+const struct smbios_entry * find_smbios_entry ( const void *start,
-			struct smbios_entry *entry ) {
+						size_t len ) {
 	static size_t offset = 0; /* Avoid repeated attempts to locate SMBIOS */
+	const struct smbios_entry *entry;
 	uint8_t sum;
 
 	/* Try to find SMBIOS */
 	for ( ; ( offset + sizeof ( *entry ) ) <= len ; offset += 0x10 ) {
 
-		/* Read start of header and verify signature */
+		/* Verify signature */
-		copy_from_user ( entry, start, offset, sizeof ( *entry ) );
+		entry = ( start + offset );
 		if ( entry->signature != SMBIOS_SIGNATURE )
 			continue;
 
+		/* Verify length */
+		if ( ( entry->len < sizeof ( *entry ) ) ||
+		     ( ( offset + entry->len ) > len ) ) {
+			DBGC ( &smbios, "SMBIOS at %#08lx has bad length "
+			       "%#02x\n", virt_to_phys ( entry ), entry->len );
+			continue;
+		}
+
 		/* Verify checksum */
-		if ( ( sum = smbios_checksum ( start, offset,
+		if ( ( sum = smbios_checksum ( entry, entry->len ) ) != 0 ) {
-					       entry->len ) ) != 0 ) {
+			DBGC ( &smbios, "SMBIOS at %#08lx has bad checksum "
-			DBG ( "SMBIOS at %08lx has bad checksum %02x\n",
+			       "%#02x\n", virt_to_phys ( entry ), sum );
-			      virt_to_phys ( start + offset ), sum );
 			continue;
 		}
 
 		/* Fill result structure */
-		DBG ( "Found SMBIOS v%d.%d entry point at %08lx\n",
+		DBGC ( &smbios, "Found SMBIOS v%d.%d entry point at %#08lx\n",
-		      entry->major, entry->minor,
+		       entry->major, entry->minor, virt_to_phys ( entry ) );
-		      virt_to_phys ( start + offset ) );
+		return entry;
-		return 0;
 	}
 
-	DBG ( "No SMBIOS found\n" );
+	DBGC ( &smbios, "No SMBIOS found\n" );
-	return -ENODEV;
+	return NULL;
 }
 
 /**
@@ -106,39 +110,45 @@ int find_smbios_entry ( userptr_t start, size_t len,
  *
  * @v start		Start address of region to scan
  * @v len		Length of region to scan
- * @v entry		SMBIOS entry point structure to fill in
+ * @ret entry		SMBIOS entry point structure, or NULL if not found
- * @ret rc		Return status code
  */
-int find_smbios3_entry ( userptr_t start, size_t len,
+const struct smbios3_entry * find_smbios3_entry ( const void *start,
-			 struct smbios3_entry *entry ) {
+						  size_t len ) {
 	static size_t offset = 0; /* Avoid repeated attempts to locate SMBIOS */
+	const struct smbios3_entry *entry;
 	uint8_t sum;
 
 	/* Try to find SMBIOS */
 	for ( ; ( offset + sizeof ( *entry ) ) <= len ; offset += 0x10 ) {
 
-		/* Read start of header and verify signature */
+		/* Verify signature */
-		copy_from_user ( entry, start, offset, sizeof ( *entry ) );
+		entry = ( start + offset );
 		if ( entry->signature != SMBIOS3_SIGNATURE )
 			continue;
 
+		/* Verify length */
+		if ( ( entry->len < sizeof ( *entry ) ) ||
+		     ( ( offset + entry->len ) > len ) ) {
+			DBGC ( &smbios, "SMBIOS at %#08lx has bad length "
+			       "%#02x\n", virt_to_phys ( entry ), entry->len );
+			continue;
+		}
+
 		/* Verify checksum */
-		if ( ( sum = smbios_checksum ( start, offset,
+		if ( ( sum = smbios_checksum ( entry, entry->len ) ) != 0 ) {
-					       entry->len ) ) != 0 ) {
+			DBGC ( &smbios, "SMBIOS3 at %#08lx has bad checksum "
-			DBG ( "SMBIOS3 at %08lx has bad checksum %02x\n",
+			       "%#02x\n", virt_to_phys ( entry ), sum );
-			      virt_to_phys ( start + offset ), sum );
 			continue;
 		}
 
 		/* Fill result structure */
-		DBG ( "Found SMBIOS3 v%d.%d entry point at %08lx\n",
+		DBGC ( &smbios, "Found SMBIOS3 v%d.%d entry point at %#08lx\n",
-		      entry->major, entry->minor,
+		       entry->major, entry->minor, virt_to_phys ( entry ) );
-		      virt_to_phys ( start + offset ) );
+		return entry;
-		return 0;
 	}
 
-	DBG ( "No SMBIOS3 found\n" );
+	DBGC ( &smbios, "No SMBIOS3 found\n" );
-	return -ENODEV;
+	return NULL;
 }
 
 /**
@@ -148,12 +158,15 @@ int find_smbios3_entry ( userptr_t start, size_t len,
  * @ret offset		Offset to strings terminator, or 0 if not found
  */
 static size_t find_strings_terminator ( size_t offset ) {
-	size_t max_offset = ( smbios.len - 2 );
+	const uint16_t *nulnul __attribute__ (( aligned ( 1 ) ));
-	uint16_t nulnul;
 
-	for ( ; offset <= max_offset ; offset++ ) {
+	/* Sanity checks */
-		copy_from_user ( &nulnul, smbios.address, offset, 2 );
+	assert ( smbios.address != NULL );
-		if ( nulnul == 0 )
+
+	/* Check for presence of terminating empty string */
+	for ( ; ( offset + sizeof ( *nulnul ) ) <= smbios.len ; offset++ ) {
+		nulnul = ( smbios.address + offset );
+		if ( *nulnul == 0 )
 			return ( offset + 1 );
 	}
 	return 0;
@@ -164,61 +177,59 @@ static size_t find_strings_terminator ( size_t offset ) {
  *
  * @v type		Structure type to search for
  * @v instance		Instance of this type of structure
- * @v structure		SMBIOS structure descriptor to fill in
+ * @ret structure	SMBIOS structure header, or NULL if not found
- * @ret rc		Return status code
  */
-int find_smbios_structure ( unsigned int type, unsigned int instance,
+const struct smbios_header * smbios_structure ( unsigned int type,
-			    struct smbios_structure *structure ) {
+						unsigned int instance ) {
+	const struct smbios_header *structure;
 	unsigned int count = 0;
 	size_t offset = 0;
 	size_t strings_offset;
 	size_t terminator_offset;
+	size_t strings_len;
 	int rc;
 
 	/* Find SMBIOS */
-	if ( ( smbios.address == UNULL ) &&
+	if ( ( smbios.address == NULL ) &&
 	     ( ( rc = find_smbios ( &smbios ) ) != 0 ) )
-		return rc;
+		return NULL;
-	assert ( smbios.address != UNULL );
+	assert ( smbios.address != NULL );
 
 	/* Scan through list of structures */
-	while ( ( ( offset + sizeof ( structure->header ) ) < smbios.len ) &&
+	while ( ( ( offset + sizeof ( *structure ) ) < smbios.len ) &&
 		( ( smbios.count == 0 ) || ( count < smbios.count ) ) ) {
 
-		/* Read next SMBIOS structure header */
+		/* Access next SMBIOS structure header */
-		copy_from_user ( &structure->header, smbios.address, offset,
+		structure = ( smbios.address + offset );
-				 sizeof ( structure->header ) );
 
 		/* Determine start and extent of strings block */
-		strings_offset = ( offset + structure->header.len );
+		strings_offset = ( offset + structure->len );
 		if ( strings_offset > smbios.len ) {
-			DBG ( "SMBIOS structure at offset %zx with length "
+			DBGC ( &smbios, "SMBIOS structure at offset %#zx "
-			      "%x extends beyond SMBIOS\n", offset,
+			       "with length %#x extends beyond SMBIOS\n",
-			      structure->header.len );
+			       offset, structure->len );
-			return -ENOENT;
+			return NULL;
 		}
 		terminator_offset = find_strings_terminator ( strings_offset );
 		if ( ! terminator_offset ) {
-			DBG ( "SMBIOS structure at offset %zx has "
+			DBGC ( &smbios, "SMBIOS structure at offset %#zx has "
-			      "unterminated strings section\n", offset );
+			       "unterminated strings section\n", offset );
-			return -ENOENT;
+			return NULL;
 		}
-		structure->strings_len = ( terminator_offset - strings_offset);
+		strings_len = ( terminator_offset - strings_offset);
-
+		DBGC ( &smbios, "SMBIOS structure at offset %#zx has type %d, "
-		DBG ( "SMBIOS structure at offset %zx has type %d, length %x, "
+		       "length %#x, strings length %#zx\n", offset,
-		      "strings length %zx\n", offset, structure->header.type,
+		       structure->type, structure->len, strings_len );
-		      structure->header.len, structure->strings_len );
 
 		/* Stop if we have reached an end-of-table marker */
 		if ( ( smbios.count == 0 ) &&
-		     ( structure->header.type == SMBIOS_TYPE_END ) )
+		     ( structure->type == SMBIOS_TYPE_END ) )
 			break;
 
 		/* If this is the structure we want, return */
-		if ( ( structure->header.type == type ) &&
+		if ( ( structure->type == type ) &&
 		     ( instance-- == 0 ) ) {
-			structure->offset = offset;
+			return structure;
-			return 0;
 		}
 
 		/* Move to next SMBIOS structure */
@@ -226,69 +237,43 @@ int find_smbios_structure ( unsigned int type, unsigned int instance,
 		count++;
 	}
 
-	DBG ( "SMBIOS structure type %d not found\n", type );
+	DBGC ( &smbios, "SMBIOS structure type %d not found\n", type );
-	return -ENOENT;
+	return NULL;
 }
 
 /**
- * Copy SMBIOS structure
+ * Get indexed string within SMBIOS structure
  *
- * @v structure		SMBIOS structure descriptor
+ * @v structure		SMBIOS structure header
- * @v data		Buffer to hold SMBIOS structure
- * @v len		Length of buffer
- * @ret rc		Return status code
- */
-int read_smbios_structure ( struct smbios_structure *structure,
-			    void *data, size_t len ) {
-
-	assert ( smbios.address != UNULL );
-
-	if ( len > structure->header.len )
-		len = structure->header.len;
-	copy_from_user ( data, smbios.address, structure->offset, len );
-	return 0;
-}
-
-/**
- * Find indexed string within SMBIOS structure
- *
- * @v structure		SMBIOS structure descriptor
  * @v index		String index
- * @v data		Buffer for string
+ * @ret string		SMBIOS string, or NULL if not fond
- * @v len		Length of string buffer
- * @ret rc		Length of string, or negative error
  */
-int read_smbios_string ( struct smbios_structure *structure,
+const char * smbios_string ( const struct smbios_header *structure,
-			 unsigned int index, void *data, size_t len ) {
+			     unsigned int index ) {
-	size_t strings_start = ( structure->offset + structure->header.len );
+	const char *string;
-	size_t strings_end = ( strings_start + structure->strings_len );
+	unsigned int i;
-	size_t offset;
+	size_t len;
-	size_t string_len;
 
-	assert ( smbios.address != UNULL );
+	/* Sanity check */
+	assert ( smbios.address != NULL );
 
-	/* String numbers start at 1 (0 is used to indicate "no string") */
+	/* Step through strings */
-	if ( ! index )
+	string = ( ( ( const void * ) structure ) + structure->len );
-		return -ENOENT;
+	for ( i = index ; i-- ; ) {
-
+		/* Get string length.  This is known safe, since we
-	for ( offset = strings_start ; offset < strings_end ;
+		 * check for the empty-string terminator in
-	      offset += ( string_len + 1 ) ) {
+		 * smbios_structure().
-		/* Get string length.  This is known safe, since the
-		 * smbios_strings struct is constructed so as to
-		 * always end on a string boundary.
 		 */
-		string_len = strlen ( smbios.address + offset );
+		len = strlen ( string );
-		if ( --index == 0 ) {
+		if ( ! len )
-			/* Copy string, truncating as necessary. */
+			break;
-			if ( len > string_len )
+		if ( i == 0 )
-				len = string_len;
+			return string;
-			copy_from_user ( data, smbios.address, offset, len );
+		string += ( len + 1 /* NUL */ );
-			return string_len;
-		}
 	}
 
-	DBG ( "SMBIOS string index %d not found\n", index );
+	DBGC ( &smbios, "SMBIOS string index %d not found\n", index );
-	return -ENOENT;
+	return NULL;
 }
 
 /**
@@ -300,10 +285,10 @@ int smbios_version ( void ) {
 	int rc;
 
 	/* Find SMBIOS */
-	if ( ( smbios.address == UNULL ) &&
+	if ( ( smbios.address == NULL ) &&
 	     ( ( rc = find_smbios ( &smbios ) ) != 0 ) )
 		return rc;
-	assert ( smbios.address != UNULL );
+	assert ( smbios.address != NULL );
 
 	return smbios.version;
 }
@@ -315,5 +300,5 @@ int smbios_version ( void ) {
 void smbios_clear ( void ) {
 
 	/* Clear address */
-	smbios.address = UNULL;
+	smbios.address = NULL;
 }

src/interface/smbios/smbios_settings.c

@@ -81,15 +81,17 @@ static int smbios_applies ( struct settings *settings __unused,
  * @v len		Length of buffer
  * @ret len		Length of setting data, or negative error
  */
-static int smbios_fetch ( struct settings *settings __unused,
+static int smbios_fetch ( struct settings *settings, struct setting *setting,
-			  struct setting *setting,
 			  void *data, size_t len ) {
-	struct smbios_structure structure;
+	const struct smbios_header *structure;
 	unsigned int tag_instance;
 	unsigned int tag_type;
 	unsigned int tag_offset;
 	unsigned int tag_len;
-	int rc;
+	const void *src;
+	size_t src_len;
+	unsigned int string;
+	union uuid uuid;
 
 	/* Split tag into instance, type, offset and length */
 	tag_instance = ( ( setting->tag >> 24 ) & 0xff );
@@ -98,81 +100,75 @@ static int smbios_fetch ( struct settings *settings __unused,
 	tag_len = ( setting->tag & 0xff );
 
 	/* Find SMBIOS structure */
-	if ( ( rc = find_smbios_structure ( tag_type, tag_instance,
+	structure = smbios_structure ( tag_type, tag_instance );
-					    &structure ) ) != 0 )
+	if ( ! structure )
-		return rc;
+		return -ENOENT;
+	src = structure;
+	src_len = structure->len;
+	string = 0;
 
-	{
+	/* A <length> of zero indicates that the byte at <offset>
-		uint8_t buf[structure.header.len];
+	 * contains a string index.  An <offset> of zero indicates
-		const void *raw;
+	 * that the <length> contains a literal string index.
-		union uuid uuid;
+	 *
-		unsigned int index;
+	 * Since the byte at offset zero can never contain a string
-
+	 * index, and a literal string index can never be zero, the
-		/* Read SMBIOS structure */
+	 * combination of both <length> and <offset> being zero
-		if ( ( rc = read_smbios_structure ( &structure, buf,
+	 * indicates that the entire structure is to be read.
-						    sizeof ( buf ) ) ) != 0 )
+	 */
-			return rc;
+	if ( ( tag_len == 0 ) && ( tag_offset == 0 ) ) {
-
+		/* Read whole structure */
-		/* A <length> of zero indicates that the byte at
+	} else if ( ( tag_len == 0 ) || ( tag_offset == 0 ) ) {
-		 * <offset> contains a string index.  An <offset> of
+		/* Read string */
-		 * zero indicates that the <length> contains a literal
+		string = tag_len;
-		 * string index.
+		if ( ( string == 0 ) && ( tag_offset < src_len ) )
-		 *
+			string = *( ( uint8_t * ) src + tag_offset );
-		 * Since the byte at offset zero can never contain a
+		src = smbios_string ( structure, string );
-		 * string index, and a literal string index can never
+		if ( ! src )
-		 * be zero, the combination of both <length> and
+			return -ENOENT;
-		 * <offset> being zero indicates that the entire
+		assert ( string > 0 );
-		 * structure is to be read.
+		src_len = strlen ( src );
-		 */
+	} else if ( tag_offset > src_len ) {
-		if ( ( tag_len == 0 ) && ( tag_offset == 0 ) ) {
+		/* Empty read beyond end of structure */
-			tag_len = sizeof ( buf );
+		src_len = 0;
-		} else if ( ( tag_len == 0 ) || ( tag_offset == 0 ) ) {
+	} else {
-			index = ( ( tag_offset == 0 ) ?
+		/* Read partial structure */
-				  tag_len : buf[tag_offset] );
+		src += tag_offset;
-			if ( ( rc = read_smbios_string ( &structure, index,
+		src_len -= tag_offset;
-							 data, len ) ) < 0 ) {
+		if ( src_len > tag_len )
-				return rc;
+			src_len = tag_len;
-			}
-			if ( ! setting->type )
-				setting->type = &setting_type_string;
-			return rc;
-		}
-
-		/* Limit length */
-		if ( tag_offset > sizeof ( buf ) ) {
-			tag_len = 0;
-		} else if ( ( tag_offset + tag_len ) > sizeof ( buf ) ) {
-			tag_len = ( sizeof ( buf ) - tag_offset );
-		}
-
-		/* Mangle UUIDs if necessary.  iPXE treats UUIDs as
-		 * being in network byte order (big-endian).  SMBIOS
-		 * specification version 2.6 states that UUIDs are
-		 * stored with little-endian values in the first three
-		 * fields; earlier versions did not specify an
-		 * endianness.  dmidecode assumes that the byte order
-		 * is little-endian if and only if the SMBIOS version
-		 * is 2.6 or higher; we match this behaviour.
-		 */
-		raw = &buf[tag_offset];
-		if ( ( ( setting->type == &setting_type_uuid ) ||
-		       ( setting->type == &setting_type_guid ) ) &&
-		     ( tag_len == sizeof ( uuid ) ) &&
-		     ( smbios_version() >= SMBIOS_VERSION ( 2, 6 ) ) ) {
-			DBG ( "SMBIOS detected mangled UUID\n" );
-			memcpy ( &uuid, &buf[tag_offset], sizeof ( uuid ) );
-			uuid_mangle ( &uuid );
-			raw = &uuid;
-		}
-
-		/* Return data */
-		if ( len > tag_len )
-			len = tag_len;
-		memcpy ( data, raw, len );
-		if ( ! setting->type )
-			setting->type = &setting_type_hex;
-		return tag_len;
 	}
+
+	/* Mangle UUIDs if necessary.  iPXE treats UUIDs as being in
+	 * network byte order (big-endian).  SMBIOS specification
+	 * version 2.6 states that UUIDs are stored with little-endian
+	 * values in the first three fields; earlier versions did not
+	 * specify an endianness.  dmidecode assumes that the byte
+	 * order is little-endian if and only if the SMBIOS version is
+	 * 2.6 or higher; we match this behaviour.
+	 */
+	if ( ( ( setting->type == &setting_type_uuid ) ||
+	       ( setting->type == &setting_type_guid ) ) &&
+	     ( src_len == sizeof ( uuid ) ) &&
+	     ( smbios_version() >= SMBIOS_VERSION ( 2, 6 ) ) ) {
+		DBGC ( settings, "SMBIOS detected mangled UUID\n" );
+		memcpy ( &uuid, src, sizeof ( uuid ) );
+		uuid_mangle ( &uuid );
+		src = &uuid;
+	}
+
+	/* Return data */
+	if ( len > src_len )
+		len = src_len;
+	memcpy ( data, src, len );
+
+	/* Set default type */
+	if ( ! setting->type ) {
+		setting->type = ( string ? &setting_type_string :
+				  &setting_type_hex );
+	}
+
+	return src_len;
 }
 
 /** SMBIOS settings operations */
@@ -192,12 +188,12 @@ static struct settings smbios_settings = {
 
 /** Initialise SMBIOS settings */
 static void smbios_init ( void ) {
+	struct settings *settings = &smbios_settings;
 	int rc;
 
-	if ( ( rc = register_settings ( &smbios_settings, NULL,
+	if ( ( rc = register_settings ( settings, NULL, "smbios" ) ) != 0 ) {
-					"smbios" ) ) != 0 ) {
+		DBGC ( settings, "SMBIOS could not register settings: %s\n",
-		DBG ( "SMBIOS could not register settings: %s\n",
+		       strerror ( rc ) );
-		      strerror ( rc ) );
 		return;
 	}
 }