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

 
 

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

Submitted by:  None
Submitted on:  Sat 20 Apr 2013 09:10:21 AM UTC  
 
Category: libfreeipmiSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Improper Behaviour
Status: FixedPrivacy: Public
Assigned to: Albert Chu <chu11>Open/Closed: Closed
Operating System: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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 File(s):
   
   
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
  • -unavailable- added by chu11 (Updated the item)
  • -unavailable- added by rswindell (Posted a comment)
  • -unavailable- added by rswindell
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 26 Apr 2013 05:43:14 PM UTCchu11StatusNone=>Fixed
      Assigned toNone=>chu11
      Open/ClosedOpen=>Closed
    Fri 26 Apr 2013 05:41:14 PM UTCchu11Attached File-=>Added 38781.patch, #27952
    Sun 21 Apr 2013 03:47:17 AM UTCrswindellCarbon-Copy-=>Added rswindell

    Back to the top


    Powered by Savane 3.1-cleanup1