[PATCH 0/2] orinoco: improve robustness of firmware load

[PATCH 0/2] orinoco: improve robustness of firmware load

[David Kilroy]

We don't currently do any verification of the firmware that we attempt
to load, and it is possible to cause a kernel oops. For example, see
<http://marc.info/?l=orinoco-users&m=123411762524637>

This series introduces a few nominal checks to reject files which are
incorrectly formatted, and then makes sure that we do not attempt to
read data outside the firmware.

It is still possible to load a duff firmware. However it shouldn't be
possible to cause any permanent damage. Hopefully it can't oops either.

Signed-off-by: David Kilroy <kilroyd@googlemail.com>
---
David Kilroy (2):
  orinoco: validate firmware header
  orinoco: prevent accessing memory outside the firmware image

 drivers/net/wireless/orinoco/fw.c         |   55 ++++++++++++++++-
 drivers/net/wireless/orinoco/hermes_dld.c |   89 +++++++++++++++--------------
 drivers/net/wireless/orinoco/hermes_dld.h |   10 ++-
 3 files changed, 103 insertions(+), 51 deletions(-)

[PATCH 1/2] orinoco: validate firmware header

[David Kilroy]

Check the Agere firmware headers for validity before attempting to
download it.

Signed-off-by: David Kilroy <kilroyd@googlemail.com>
---
 drivers/net/wireless/orinoco/fw.c |   45 +++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/orinoco/fw.c b/drivers/net/wireless/orinoco/fw.c
index 7d2292d..b2ad24f 100644
--- a/drivers/net/wireless/orinoco/fw.c
+++ b/drivers/net/wireless/orinoco/fw.c
@@ -28,6 +28,16 @@ static const struct fw_info orinoco_fw[] = {
 	{ "symbol_sp24t_prim_fw", "symbol_sp24t_sec_fw", NULL, 0x00003100, 512 }
 };
 
+static const char *fw_err[] = {
+	"image too small",
+	"format not recognised",
+	"bad headersize",
+	"bad block offset",
+	"bad PDR offset",
+	"bad PRI offset",
+	"bad compat offset"
+};
+
 /* Structure used to access fields in FW
  * Make sure LE decoding macros are used
  */
@@ -43,6 +53,32 @@ struct orinoco_fw_header {
 	char signature[0];      /* FW signature length headersize-20 */
 } __attribute__ ((packed));
 
+/* Check the range of various header entries */
+static int validate_fw(const struct orinoco_fw_header *hdr, size_t len)
+{
+	u16 hdrsize;
+
+	if (len < sizeof(*hdr))
+		return 1;
+	if (memcmp(hdr->hdr_vers, "HFW", 3) != 0)
+		return 2;
+
+	hdrsize = le16_to_cpu(hdr->headersize);
+	if (hdrsize > len)
+		return 3;
+	if ((hdrsize + le32_to_cpu(hdr->block_offset)) > len)
+		return 4;
+	if ((hdrsize + le32_to_cpu(hdr->pdr_offset)) > len)
+		return 5;
+	if ((hdrsize + le32_to_cpu(hdr->pri_offset)) > len)
+		return 6;
+	if ((hdrsize + le32_to_cpu(hdr->compat_offset)) > len)
+		return 7;
+
+	/* TODO: consider adding a checksum or CRC to the firmware format */
+	return 0;
+}
+
 /* Download either STA or AP firmware into the card. */
 static int
 orinoco_dl_firmware(struct orinoco_private *priv,
@@ -93,6 +129,15 @@ orinoco_dl_firmware(struct orinoco_private *priv,
 
 	hdr = (const struct orinoco_fw_header *) fw_entry->data;
 
+	err = validate_fw(hdr, fw_entry->size);
+	if (err) {
+		printk(KERN_WARNING "%s: Invalid firmware image detected (%s). "
+		       "Aborting download\n",
+		       dev->name, fw_err[err - 1]);
+		err = -EINVAL;
+		goto abort;
+	}
+
 	/* Enable aux port to allow programming */
 	err = hermesi_program_init(hw, le32_to_cpu(hdr->entry_point));
 	printk(KERN_DEBUG "%s: Program init returned %d\n", dev->name, err);
-- 
1.6.0.6

Re: [Orinoco-devel] [PATCH 1/2] orinoco: validate firmware header

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On 20 of February 2009 02:46:26 David Kilroy wrote:
>
> +static const char *fw_err[] = {
> +	"image too small",
> +	"format not recognised",
> +	"bad headersize",
> +	"bad block offset",
> +	"bad PDR offset",
> +	"bad PRI offset",
> +	"bad compat offset"
> +};
> +
>  /* Structure used to access fields in FW
>   * Make sure LE decoding macros are used
>   */
> @@ -43,6 +53,32 @@ struct orinoco_fw_header {
>  	char signature[0];      /* FW signature length headersize-20 */
>  } __attribute__ ((packed));
>
> +/* Check the range of various header entries */
> +static int validate_fw(const struct orinoco_fw_header *hdr, size_t
> len) +{
> +	u16 hdrsize;
> +
> +	if (len < sizeof(*hdr))
> +		return 1;
> +	if (memcmp(hdr->hdr_vers, "HFW", 3) != 0)
> +		return 2;
> +
> +	hdrsize = le16_to_cpu(hdr->headersize);
> +	if (hdrsize > len)
> +		return 3;
> +	if ((hdrsize + le32_to_cpu(hdr->block_offset)) > len)
> +		return 4;
> +	if ((hdrsize + le32_to_cpu(hdr->pdr_offset)) > len)
> +		return 5;
> +	if ((hdrsize + le32_to_cpu(hdr->pri_offset)) > len)
> +		return 6;
> +	if ((hdrsize + le32_to_cpu(hdr->compat_offset)) > len)
> +		return 7;
> +
> +	/* TODO: consider adding a checksum or CRC to the firmware format
> */ +	return 0;
> +}


I am afraid this can easily go off sync. Any reason those messages 
cannot be printed inline in validate_fw()?

Otherwise what about

#define FW_ERR_OK	0
#define FW_ERR_TOO_SMALL 1
...
static const char *fw_err[] = {
	[FW_ERR_TOO_SMALL] = "image too small",
...

	if (len < sizeof(*hdr))
		return FW_ERR_TOO_SMALL;
...
	return FW_ERR_OK;

?

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Orinoco-devel] [PATCH 1/2] orinoco: validate firmware header

[Dave]

Andrey Borzenkov wrote:
> On 20 of February 2009 02:46:26 David Kilroy wrote:
>> +static const char *fw_err[] = {
>> +	"image too small",
>> +};
>> +
>>
>> +/* Check the range of various header entries */
>> +static int validate_fw(const struct orinoco_fw_header *hdr, size_t
>> len) +{
>> +	if (len < sizeof(*hdr))
>> +		return 1;
>> +
>> +	/* TODO: consider adding a checksum or CRC to the firmware format
>> */ +	return 0;
>> +}
> 
> 
> I am afraid this can easily go off sync. Any reason those messages 
> cannot be printed inline in validate_fw()?

That's how I started, but I didn't like having to pass down struct
net_device/struct orinoco_private just to print an error.

> Otherwise what about
> 
> #define FW_ERR_OK	0
> #define FW_ERR_TOO_SMALL 1
> ...
> static const char *fw_err[] = {
> 	[FW_ERR_TOO_SMALL] = "image too small",
> ?

Sure. That (or an enumeration) documents the error codes. A little
tedious though. What if I did something like:

static char * validate_fw(...)
{
   if (len < sizeof(*hdr))
      return "image too small";
   ...

   return NULL;
}

...
{
   char *fw_err = validate_firmware(hdr, fw_entry->size);

   if (fw_err != NULL) {
       printk(KERN_WARNING "%s: Invalid firmware (%s)\n",
	      dev->name, fw_err)
       err = -EINVAL;
       goto abort;
   }
}


Dave.

[PATCH 2/2] orinoco: prevent accessing memory outside the firmware image

[David Kilroy]

Do this by indicating the end of the appropriate regions of memory.

Note that MAX_PDA_SIZE should only apply to the PDA block read from
flash/EEPROM, and has been erronously applied to the pdr elements.
Remove the macro, and use the actual PDA size passed down by the caller.

We also fix up some of the types used, marking as much as possible
const, and using void* for the end pointers.

Signed-off-by: David Kilroy <kilroyd@googlemail.com>
---
 drivers/net/wireless/orinoco/fw.c         |   10 ++-
 drivers/net/wireless/orinoco/hermes_dld.c |   89 +++++++++++++++--------------
 drivers/net/wireless/orinoco/hermes_dld.h |   10 ++-
 3 files changed, 58 insertions(+), 51 deletions(-)

diff --git a/drivers/net/wireless/orinoco/fw.c b/drivers/net/wireless/orinoco/fw.c
index b2ad24f..4410edd 100644
--- a/drivers/net/wireless/orinoco/fw.c
+++ b/drivers/net/wireless/orinoco/fw.c
@@ -92,7 +92,7 @@ orinoco_dl_firmware(struct orinoco_private *priv,
 	const struct firmware *fw_entry;
 	const struct orinoco_fw_header *hdr;
 	const unsigned char *first_block;
-	const unsigned char *end;
+	const void *end;
 	const char *firmware;
 	struct net_device *dev = priv->ndev;
 	int err = 0;
@@ -160,7 +160,8 @@ orinoco_dl_firmware(struct orinoco_private *priv,
 		       le16_to_cpu(hdr->headersize) +
 		       le32_to_cpu(hdr->pdr_offset));
 
-	err = hermes_apply_pda_with_defaults(hw, first_block, pda);
+	err = hermes_apply_pda_with_defaults(hw, first_block, end, pda,
+					     &pda[fw->pda_size / sizeof(*pda)]);
 	printk(KERN_DEBUG "%s: Apply PDA returned %d\n", dev->name, err);
 	if (err)
 		goto abort;
@@ -192,7 +193,7 @@ free:
  */
 static int
 symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
-		const unsigned char *image, const unsigned char *end,
+		const unsigned char *image, const void *end,
 		int secondary)
 {
 	hermes_t *hw = &priv->hw;
@@ -235,7 +236,8 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
 	if (secondary) {
 		size_t len = hermes_blocks_length(first_block);
 		ptr = first_block + len;
-		ret = hermes_apply_pda(hw, ptr, pda);
+		ret = hermes_apply_pda(hw, ptr, end, pda,
+				       &pda[fw->pda_size / sizeof(*pda)]);
 		kfree(pda);
 		if (ret)
 			return ret;
diff --git a/drivers/net/wireless/orinoco/hermes_dld.c b/drivers/net/wireless/orinoco/hermes_dld.c
index 5260ceb..aaf8c68 100644
--- a/drivers/net/wireless/orinoco/hermes_dld.c
+++ b/drivers/net/wireless/orinoco/hermes_dld.c
@@ -71,18 +71,6 @@
 #define BLOCK_END	0xFFFFFFFF	/* Last image block */
 #define TEXT_END	0x1A		/* End of text header */
 
-/*
- * PDA == Production Data Area
- *
- * In principle, the max. size of the PDA is is 4096 words. Currently,
- * however, only about 500 bytes of this area are used.
- *
- * Some USB implementations can't handle sizes in excess of 1016. Note
- * that PDA is not actually used in those USB environments, but may be
- * retrieved by common code.
- */
-#define MAX_PDA_SIZE	1000
-
 /* Limit the amout we try to download in a single shot.
  * Size is in bytes.
  */
@@ -218,13 +206,14 @@ hermes_aux_control(hermes_t *hw, int enabled)
  * Scan PDR for the record with the specified RECORD_ID.
  * If it's not found, return NULL.
  */
-static struct pdr *
-hermes_find_pdr(struct pdr *first_pdr, u32 record_id)
+static const struct pdr *
+hermes_find_pdr(const struct pdr *first_pdr, u32 record_id, const void *end)
 {
-	struct pdr *pdr = first_pdr;
-	void *end = (void *)first_pdr + MAX_PDA_SIZE;
+	const struct pdr *pdr = first_pdr;
 
-	while (((void *)pdr < end) &&
+	end -= sizeof(struct pdr);
+
+	while (((void *)pdr <= end) &&
 	       (pdr_id(pdr) != PDI_END)) {
 		/*
 		 * PDR area is currently not terminated by PDI_END.
@@ -244,12 +233,15 @@ hermes_find_pdr(struct pdr *first_pdr, u32 record_id)
 }
 
 /* Scan production data items for a particular entry */
-static struct pdi *
-hermes_find_pdi(struct pdi *first_pdi, u32 record_id)
+static const struct pdi *
+hermes_find_pdi(const struct pdi *first_pdi, u32 record_id, const void *end)
 {
-	struct pdi *pdi = first_pdi;
+	const struct pdi *pdi = first_pdi;
+
+	end -= sizeof(struct pdi);
 
-	while (pdi_id(pdi) != PDI_END) {
+	while (((void *)pdi <= end) &&
+	       (pdi_id(pdi) != PDI_END)) {
 
 		/* If the record ID matches, we are done */
 		if (pdi_id(pdi) == record_id)
@@ -262,12 +254,13 @@ hermes_find_pdi(struct pdi *first_pdi, u32 record_id)
 
 /* Process one Plug Data Item - find corresponding PDR and plug it */
 static int
-hermes_plug_pdi(hermes_t *hw, struct pdr *first_pdr, const struct pdi *pdi)
+hermes_plug_pdi(hermes_t *hw, const struct pdr *first_pdr,
+		const struct pdi *pdi, const void *pdr_end)
 {
-	struct pdr *pdr;
+	const struct pdr *pdr;
 
 	/* Find the PDR corresponding to this PDI */
-	pdr = hermes_find_pdr(first_pdr, pdi_id(pdi));
+	pdr = hermes_find_pdr(first_pdr, pdi_id(pdi), pdr_end);
 
 	/* No match is found, safe to ignore */
 	if (!pdr)
@@ -345,18 +338,22 @@ int hermes_read_pda(hermes_t *hw,
  */
 int hermes_apply_pda(hermes_t *hw,
 		     const char *first_pdr,
-		     const __le16 *pda)
+		     const void *pdr_end,
+		     const __le16 *pda,
+		     const void *pda_end)
 {
 	int ret;
 	const struct pdi *pdi;
-	struct pdr *pdr;
+	const struct pdr *pdr;
 
-	pdr = (struct pdr *) first_pdr;
+	pdr = (const struct pdr *) first_pdr;
+	pda_end -= sizeof(struct pdi);
 
 	/* Go through every PDI and plug them into the adapter */
 	pdi = (const struct pdi *) (pda + 2);
-	while (pdi_id(pdi) != PDI_END) {
-		ret = hermes_plug_pdi(hw, pdr, pdi);
+	while (((void *)pdi <= pda_end) &&
+	       (pdi_id(pdi) != PDI_END)) {
+		ret = hermes_plug_pdi(hw, pdr, pdi, pdr_end);
 		if (ret)
 			return ret;
 
@@ -476,7 +473,7 @@ int hermesi_program_end(hermes_t *hw)
 }
 
 /* Program the data blocks */
-int hermes_program(hermes_t *hw, const char *first_block, const char *end)
+int hermes_program(hermes_t *hw, const char *first_block, const void *end)
 {
 	const struct dblock *blk;
 	u32 blkaddr;
@@ -488,14 +485,14 @@ int hermes_program(hermes_t *hw, const char *first_block, const char *end)
 
 	blk = (const struct dblock *) first_block;
 
-	if ((const char *) blk > (end - sizeof(*blk)))
+	if ((void *) blk > (end - sizeof(*blk)))
 		return -EIO;
 
 	blkaddr = dblock_addr(blk);
 	blklen = dblock_len(blk);
 
 	while ((blkaddr != BLOCK_END) &&
-	       (((const char *) blk + blklen) <= end)) {
+	       (((void *) blk + blklen) <= end)) {
 		printk(KERN_DEBUG PFX
 		       "Programming block of length %d to address 0x%08x\n",
 		       blklen, blkaddr);
@@ -527,7 +524,7 @@ int hermes_program(hermes_t *hw, const char *first_block, const char *end)
 #endif
 		blk = (const struct dblock *) &blk->data[blklen];
 
-		if ((const char *) blk > (end - sizeof(*blk)))
+		if ((void *) blk > (end - sizeof(*blk)))
 			return -EIO;
 
 		blkaddr = dblock_addr(blk);
@@ -545,9 +542,9 @@ static const struct {							\
 	__le16 id;							\
 	u8 val[length];							\
 } __attribute__ ((packed)) default_pdr_data_##pid = {			\
-	cpu_to_le16((sizeof(default_pdr_data_##pid)/		\
+	cpu_to_le16((sizeof(default_pdr_data_##pid)/			\
 				sizeof(__le16)) - 1),			\
-	cpu_to_le16(pid),					\
+	cpu_to_le16(pid),						\
 	data								\
 }
 
@@ -616,17 +613,20 @@ DEFINE_DEFAULT_PDR(0x0161, 256,
  */
 int hermes_apply_pda_with_defaults(hermes_t *hw,
 				   const char *first_pdr,
-				   const __le16 *pda)
+				   const void *pdr_end,
+				   const __le16 *pda,
+				   const void *pda_end)
 {
 	const struct pdr *pdr = (const struct pdr *) first_pdr;
-	struct pdi *first_pdi = (struct pdi *) &pda[2];
-	struct pdi *pdi;
-	struct pdi *default_pdi = NULL;
-	struct pdi *outdoor_pdi;
-	void *end = (void *)first_pdr + MAX_PDA_SIZE;
+	const struct pdi *first_pdi = (const struct pdi *) &pda[2];
+	const struct pdi *pdi;
+	const struct pdi *default_pdi = NULL;
+	const struct pdi *outdoor_pdi;
 	int record_id;
 
-	while (((void *)pdr < end) &&
+	pdr_end -= sizeof(struct pdr);
+
+	while (((void *)pdr <= pdr_end) &&
 	       (pdr_id(pdr) != PDI_END)) {
 		/*
 		 * For spectrum_cs firmwares,
@@ -638,7 +638,7 @@ int hermes_apply_pda_with_defaults(hermes_t *hw,
 			break;
 		record_id = pdr_id(pdr);
 
-		pdi = hermes_find_pdi(first_pdi, record_id);
+		pdi = hermes_find_pdi(first_pdi, record_id, pda_end);
 		if (pdi)
 			printk(KERN_DEBUG PFX "Found record 0x%04x at %p\n",
 			       record_id, pdi);
@@ -646,7 +646,8 @@ int hermes_apply_pda_with_defaults(hermes_t *hw,
 		switch (record_id) {
 		case 0x110: /* Modem REFDAC values */
 		case 0x120: /* Modem VGDAC values */
-			outdoor_pdi = hermes_find_pdi(first_pdi, record_id + 1);
+			outdoor_pdi = hermes_find_pdi(first_pdi, record_id + 1,
+						      pda_end);
 			default_pdi = NULL;
 			if (outdoor_pdi) {
 				pdi = outdoor_pdi;
diff --git a/drivers/net/wireless/orinoco/hermes_dld.h b/drivers/net/wireless/orinoco/hermes_dld.h
index 6fcb262..9881f08 100644
--- a/drivers/net/wireless/orinoco/hermes_dld.h
+++ b/drivers/net/wireless/orinoco/hermes_dld.h
@@ -29,7 +29,7 @@
 
 int hermesi_program_init(hermes_t *hw, u32 offset);
 int hermesi_program_end(hermes_t *hw);
-int hermes_program(hermes_t *hw, const char *first_block, const char *end);
+int hermes_program(hermes_t *hw, const char *first_block, const void *end);
 
 int hermes_read_pda(hermes_t *hw,
 		    __le16 *pda,
@@ -38,10 +38,14 @@ int hermes_read_pda(hermes_t *hw,
 		    int use_eeprom);
 int hermes_apply_pda(hermes_t *hw,
 		     const char *first_pdr,
-		     const __le16 *pda);
+		     const void *pdr_end,
+		     const __le16 *pda,
+		     const void *pda_end);
 int hermes_apply_pda_with_defaults(hermes_t *hw,
 				   const char *first_pdr,
-				   const __le16 *pda);
+				   const void *pdr_end,
+				   const __le16 *pda,
+				   const void *pda_end);
 
 size_t hermes_blocks_length(const char *first_block);
 
-- 
1.6.0.6

Re: [PATCH 2/2] orinoco: prevent accessing memory outside the firmware image

[Dave]

David Kilroy wrote:
> Do this by indicating the end of the appropriate regions of memory.
> 
> Note that MAX_PDA_SIZE should only apply to the PDA block read from
> flash/EEPROM, and has been erronously applied to the pdr elements.
> Remove the macro, and use the actual PDA size passed down by the caller.
> 
> We also fix up some of the types used, marking as much as possible
> const, and using void* for the end pointers.
> 
> Signed-off-by: David Kilroy <kilroyd@googlemail.com>
> ---

I've missed (at least) a couple places where I need to add checks:

 * When looking for PDR for Symbol firmware.
 * When applying PDR data I need to check the data is within the FW image

I'll update this patch, address Andreys comments on the other, and resubmit.


Dave.