[Freeipmi-devel] Re: [llnl-devel] checking chksums, possible framework change needed??

[Anand Babu]

I always kept changing the order of chksum and comp_code
validation. And I decided to postpone it until disaster strikes again.

Let us fix it now.

Validity of the comp_code itself depends on the chksum. But location
of chksum depends on comp_code. There seems to be a cyclic dependency.

I think it is OK, not to validate the chksum if comp_code is not
success. In that case we can check comp_code first.
OR
We can double-check the chksum and then validate comp_code
like this

if (pkt.chksum != ipmi_chksum((u_int8_t *) &pkt.msg.rs,
                  (sizeof (pkt.msg.rs) + sizeof(pkt.data))))
{
  ipmi_err_rs_t err_rs;
  memcpy (&err_rs, &pkt.msg.rs, sizeof (ipmi_err_rs_t));
  if (err_rs.chksum != ipmi_chksum((u_int8_t *) &err_rs.msg.rs,
                  (sizeof (err_rs.msg.rs) + sizeof(err_rs.data))))
  {
    REALLY BAD CHKSUM
  }  
}
AND THEN VALIDATE COMP_CODE

where

typedef struct ipmi_err_rs
{
  ipmi_session_t session;
  ipmi_rs_msg_t msg;
  struct {
  // just for writing generic chksum macros
  } data;
  ipmi_chksum_t chksum;
} ipmi_err_rs_t;

There is also another approach using "c union", but too may nesting of
structures aren't elegant.

Happy Hacking
-ab

Albert Chu <address@hidden> writes:

>Hey AB,
>
>In your ipmi-wrapper.c code, you do checksum checks like the following:
>
>if (pkt.chksum != ipmi_chksum((u_int8_t *) &pkt.msg.rs,
>                  (sizeof (pkt.msg.rs) + sizeof(pkt.data))))
>{
>     // yell and scream about checksum errors
>}
>
>However this is incorrect.  The reason is that on a completion code
>error (for example, a bad username or bad password error), response data
>may not be returned.  Thus, the packet received will be smaller than
>expected.  Thus, the response packet's checksum field may not be copied
>into the pkt.chksum field properly.  And the above chksum check bombs.
>
>For example, a get_session_challenge_response is 42 bytes (rmcp +
>session + rs_msg + data + chksum).  However, if you passed a bad
>username during the get_session_challenge_request command, the response
>will only be 23 bytes (the only response data is the completion code, no
>session_id or challenge string is returned).  Therefore, calling
>ipmi_chksum using your above example will fail, when in fact, it
>shouldn't fail.  The checksums are probably fine, its the completion
>code that is the problem.
>
>I think the correct way is to do this is as follows:
>
>len = ipmi_recvfrom(fd, &rmcp_hdr, &ipmi_pkt, sizeof(ipmi_pkt));
>if (len <= 0)
>   // blah blah
>
>if (ipmi_response_chksum(&ipmi_pkt.msg.rs, len) == 0)
>   // blah blah yell and scream
>
>I think the ipmi_recvfrom should return the length of data stored
>in the ipmi_pkt buffer.  Then, the ipmi_chksum_response function
>assumes the last byte of the buffer passed in is the checksum field.
>
>Let me know what you think ...
>
>Al
>
>--
>Albert Chu
>address@hidden
>Lawrence Livermore National Laboratories
>
>
>_______________________________________________
>llnl-devel mailing list
>address@hidden
>http://californiadigital.com/cgi-bin/mailman/listinfo/llnl-devel

--
Anand Babu
CaliforniaDigital.com
Office# +1-510-687-7045
Cell# +1-510-396-0717
Home# +1-510-894-0586

Free as in Freedom <www.gnu.org>