bugGNU FreeIPMI - Bugs: bug #38781, Errant "Legacy PAD" byte...

 
 

bug #38781: Errant "Legacy PAD" byte added to IPMI 2.0 (RMCP+) packets

Submitter:  None
Submitted:  Sat 20 Apr 2013 09:10:21 AM UTC
   
 
Category:  libfreeipmi Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Improper Behaviour
Status:  Fixed Privacy:  Public
Assigned to:  chu11 Open/Closed:  Closed
Operating System:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 08 May 2013 11:11:10 PM UTC, comment #7: 

I agree that it's legacy enough that it may not be worth keeping around.  I looked around at other open source IPMI projects and found that both ipmitool and openipmi do not do the pad (ipmiutil does though).

I'll remove it for FreeIPMI 1.3.1.

Albert Chu <chu11>
Group administrator
Tue 07 May 2013 11:51:19 PM UTC, comment #6: 

Just so you are aware, the still-existing code in ipmi_lan_sendto() is still incorrect for IPMI 1.5 compatibility:

  /*
    Note from Table 12-8, RMCP Packet for IPMI via Ethernet footnote
    Some LAN adapter chips may have a problem where packets of overall
    lengths 56, 84, 112, 128, or 156 are not handled correctly. The
    PAD byte is added as necessary to avoid these overall
    lengths. Remote console software must use the PAD byte when
    formatting packets to any 10/100 Ethernet device that accepts RMCP
    packets. -- Anand Babu
  */
  _len = len;
  if (_len == 56
      || _len == 84
      || _len == 112
      || _len == 128
      || _len == 156)
    pad_len += IPMI_LAN_PKT_PAD_SIZE;

Note the words "overall length" are used here in the comment (and in the specification), yet the length value being compared here is only the UDP payload length (not the "overall length").

If you really want this code to be correct, you would need to calculate the "overall length" of the Ethernet frame, which includes Ethernet frame header and CRC, IP header, and UDP header. Or perhaps the "overall length" only includes the UDP/IP portion - the specification just says "packets of overall lengths", so that's a little bit vague.

My guess is that this "Legacy PAD" probably isn't needed by whatever broken network controllers required it 10+ years ago and since FreeIPMI isn't padding the correct packets anyway, I suggest just removing this code. It's wrong.

-Rob

Rob Swindell <rswindell>
Fri 26 Apr 2013 05:41:14 PM UTC, comment #5: 

I fixed this by creating a new ipmi_rmcpplus_sendto and ipmi_rmcpplus_recvfrom functions and updating things appropriately in libfreeipmi, libipmiconsole, and ipmipower.

The patch I applied to the stable branch is attached.



(file #27952)

Albert Chu <chu11>
Group administrator
Thu 25 Apr 2013 01:54:23 AM UTC, comment #4: 

Here's the patch (another one-liner):

Index: libfreeipmi/api/ipmi-lan-session-common.c
===================================================================
--- libfreeipmi/api/ipmi-lan-session-common.c   (revision 9608)
+++ libfreeipmi/api/ipmi-lan-session-common.c   (working copy)
@@ -2304,7 +2304,10 @@

   do
     {
-      ret = ipmi_lan_sendto (ctx->io.outofband.sockfd,
+      /* bug #38781 fix:
+         Do not use ipmi_lan_sendto() for RMCP+ packets as the "Legacy Pad" byte added by that function
+         is not needed or allowed with IPMI 2.0/RMCP+ */
+      ret = sendto (ctx->io.outofband.sockfd,
                              pkt,
                              send_len,
                              0,

Rob Swindell <rswindell>
Sun 21 Apr 2013 12:05:01 AM UTC, comment #3: 

Never mind about the cipher-suite 3 comment, that is a different bug where the "Confidentiality Pad Length" value is 16 (0x10):

192.168.1.19: [  BYTE ARRAY ... ] = confidentiality_trailer[17B]
192.168.1.19: [ 01h 02h 03h 04h 05h 06h 07h 08h ]
192.168.1.19: [ 09h 0Ah 0Bh 0Ch 0Dh 0Eh 0Fh 10h ]
192.168.1.19: [ 10h ]

Which clearly violates IPMI 2.0 (table 13-20) which states the Confidentiality Pad Length value can be from "0 to 15".

I'll file a different bug for that one.

-Rob Swindell

Anonymous
Sat 20 Apr 2013 11:25:59 PM UTC, comment #2: 

Another example command (using cipher-suite 3) which demonstrates this bug:

ipmi-dcmi -h 192.168.1.19 --set-asset-tag=123456789012345678901 -Dlan20 -I3

Anonymous
Sat 20 Apr 2013 09:20:38 AM UTC, comment #1: 

The example command-line should've included '-Dlan20' and '-I2'. I realize that these may not be some people's default values in their freeeipmi.conf file.

Anonymous
Sat 20 Apr 2013 09:10:21 AM UTC, original submission:  

In ipmi-lan-interface.c->ipmi_lan_sendto(), the following code exists:

/*
    Note from Table 12-8, RMCP Packet for IPMI via Ethernet footnote
    Some LAN adapter chips may have a problem where packets of overall
    lengths 56, 84, 112, 128, or 156 are not handled correctly. The
    PAD byte is added as necessary to avoid these overall
    lengths. Remote console software must use the PAD byte when
    formatting packets to any 10/100 Ethernet device that accepts RMCP
    packets. -- Anand Babu
  */
  _len = len;
  if (_len == 56
      || _len == 84
      || _len == 112
      || _len == 128
      || _len == 156)
    pad_len += IPMI_LAN_PKT_PAD_SIZE;

The problem is that this code should only apply to IPMI 1.5 packets and should not apply to IPMI 2.0 (RMCP+) packets. This code causes integrity (AuthCode) check failures when used with IPMI 2.0 servers (BMCs) when the packet size happens to be one of the "magic" lengths shown above.

You can observe this problem with the following example command (and many others):

ipmi-dcmi -h 192.168.1.19 --set-asset-tag=1234567890123

The solution is to either:
1. remove this code
2. create another ipmi_lan2_sendto() function which doesn't perform this padding
3. add an argument to ipmi_lan_sendto() to defeat this behavior when called from _api_lan_2_0_cmd_send().

-Rob Swindell

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27952:  38781.patch added by chu11 (14KiB - text/x-patch - Patch applied to stable 1.2.X branch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by chu11 (Updated the item)
  • -email is unavailable- added by rswindell (Posted a comment)
  • -email is unavailable- added by rswindell
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-04-26 chu11 StatusNone Fixed
        Assigned toNone chu11
        Open/ClosedOpen Closed
    2013-04-26 chu11 Attached File- Added 38781.patch, #27952
    2013-04-21 rswindell Carbon-Copy- Added rswindell

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code