[PATCH openpower-host-ipmi-oem v3] Fix endianness issue5

[PATCH openpower-host-ipmi-oem v3] Fix endianness issue5

[OpenBMC Patches]

Make it safe in both little-endian and big-endian bmc chip.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/openbmc/openpower-host-ipmi-oem/13)
<!-- Reviewable:end -->


https://github.com/openbmc/openpower-host-ipmi-oem/pull/13

Nan Li (1):
  Fix issue5 for endian safe

 oemhandler.C | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.8.3

[PATCH openpower-host-ipmi-oem v3] Fix issue5 for endian safe

[OpenBMC Patches]

From: Nan Li <bjlinan@cn.ibm.com>

Make recored id and offset endian-safe.
Use le16toh() of <endian.h>

Signed-off-by: Nan Li <bjlinan@cn.ibm.com>
---
 oemhandler.C | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/oemhandler.C b/oemhandler.C
index 026e0d1..dc9ae53 100644
--- a/oemhandler.C
+++ b/oemhandler.C
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <systemd/sd-bus.h>
+#include <endian.h>
 
 void register_netfn_oem_partial_esel() __attribute__((constructor));
 
@@ -29,9 +30,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 	esel_request_t *reqptr = (esel_request_t*) request;
 	FILE *fp;
 	int r = 0;
-	// TODO: Issue 5: This is not endian-safe.
-	short *recid  =  (short*) &reqptr->selrecordls;
-	short *offset =  (short*) &reqptr->offsetls;
+	short recid  =  le16toh((((unsigned short) reqptr->selrecordms) << 8) + reqptr->selrecordls);
+	short offset =  le16toh((((unsigned short) reqptr->offsetms) << 8) + reqptr->offsetls);
 	uint8_t rlen;
 	ipmi_ret_t rc = IPMI_CC_OK;
 	const char *pio;
@@ -59,7 +59,7 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 
     // OpenPOWER Host Interface spec says if RecordID and Offset are
 	// 0 then then this is a new request
-	if (!*recid && !*offset)
+	if (!recid && !offset)
 		pio = "wb";
 	else
 		pio = "rb+";
@@ -67,10 +67,10 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 	rlen = (*data_len) - (uint8_t) (sizeof(esel_request_t));
 
 	printf("IPMI PARTIAL ESEL for %s  Offset = %d Length = %d\n",
-		g_esel_path, *offset, rlen);
+		g_esel_path, offset, rlen);
 
 	if ((fp = fopen(g_esel_path, pio)) != NULL) {
-		fseek(fp, *offset, SEEK_SET);
+		fseek(fp, offset, SEEK_SET);
 		fwrite(reqptr+1,rlen,1,fp);
 		fclose(fp);
 
-- 
2.8.3

Re: [PATCH openpower-host-ipmi-oem v3] Fix issue5 for endian safe

[Jeremy Kerr]

Hi Nan,

> @@ -29,9 +30,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  	esel_request_t *reqptr = (esel_request_t*) request;
>  	FILE *fp;
>  	int r = 0;
> -	// TODO: Issue 5: This is not endian-safe.
> -	short *recid  =  (short*) &reqptr->selrecordls;
> -	short *offset =  (short*) &reqptr->offsetls;
> +	short recid  =  le16toh((((unsigned short) reqptr->selrecordms) << 8) + reqptr->selrecordls);
> +	short offset =  le16toh((((unsigned short) reqptr->offsetms) << 8) + reqptr->offsetls);

I think that this is less correct than the first attempt, and that's
probably due to an incorrect suggestion from me. Sorry for that.

I've only just noticed that you're using different members of the reqptr
struct to construct that 16-bit value; so your first patch probably did
the right thing.

However: we should fix the actual source of the problem - that the
fields in struct esel_request_t are unnecessarily split into uint8_t
fields.

I think it'd be much better if we changed esel_request_t to this:

  struct esel_request_t {
      uint16_t  resid;
      uint16_t  selrecord;
      uint16_t  offset;
      uint8_t  progress;
  }  __attribute__ ((packed)) ;

- and did the proper conversion with le16toh().

[Also, the typo in the commit title is still present]

Regards,


Jeremy

Re: [PATCH openpower-host-ipmi-oem v3] Fix issue5 for endian safe

[Nan KX Li]

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

Hi Jeremy,

I've updated my pull request : 
https://github.com/openbmc/openpower-host-ipmi-oem/pull/13.
Please review it.

For the alignment requirement, I'd use unsigned char type to access the 
requestor's buffer. So:

uint8_t *reqptr = (uint8_t *) request; 

esel_req.resid     = le16toh((((uint16_t) reqptr[1]) << 8) + reqptr[0]); 
esel_req.selrecord = le16toh((((uint16_t) reqptr[3]) << 8) + reqptr[2]); 
esel_req.offset    = le16toh((((uint16_t) reqptr[5]) << 8) + reqptr[4]); 



Regards,

William Li ( Li Nan, 李楠 ) 
 

Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605

Beijing, China



From:   Jeremy Kerr <jk@ozlabs.org>
To:     OpenBMC Patches <openbmc-patches@stwcx.xyz>, 
openbmc@lists.ozlabs.org
Cc:     Nan KX Li/China/IBM@IBMCN
Date:   05/30/2016 14:08
Subject:        Re: [PATCH openpower-host-ipmi-oem v3] Fix issue5 for 
endian safe



Hi Nan,

> @@ -29,9 +30,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t 
netfn, ipmi_cmd_t cmd,
>                esel_request_t *reqptr = (esel_request_t*) request;
>                FILE *fp;
>                int r = 0;
> -              // TODO: Issue 5: This is not endian-safe.
> -              short *recid  =  (short*) &reqptr->selrecordls;
> -              short *offset =  (short*) &reqptr->offsetls;
> +              short recid  =  le16toh((((unsigned short) 
reqptr->selrecordms) << 8) + reqptr->selrecordls);
> +              short offset =  le16toh((((unsigned short) 
reqptr->offsetms) << 8) + reqptr->offsetls);

I think that this is less correct than the first attempt, and that's
probably due to an incorrect suggestion from me. Sorry for that.

I've only just noticed that you're using different members of the reqptr
struct to construct that 16-bit value; so your first patch probably did
the right thing.

However: we should fix the actual source of the problem - that the
fields in struct esel_request_t are unnecessarily split into uint8_t
fields.

I think it'd be much better if we changed esel_request_t to this:

  struct esel_request_t {
      uint16_t  resid;
      uint16_t  selrecord;
      uint16_t  offset;
      uint8_t  progress;
  }  __attribute__ ((packed)) ;

- and did the proper conversion with le16toh().

[Also, the typo in the commit title is still present]

Regards,


Jeremy





[-- Attachment #2: Type: text/html, Size: 4555 bytes --]