netdev - Re: lib/packing.c behaving weird if buffer length is not multiple of 4 with QUIRK_LSW32_IS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9170351b-3038-419a-8414-fe8513a5bb57@intel.com>
Date: Thu, 22 Aug 2024 18:41:57 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: lib/packing.c behaving weird if buffer length is not multiple of
 4 with QUIRK_LSW32_IS_FIRST



On 8/21/2024 4:41 PM, Jacob Keller wrote:
> 
> 
> On 8/21/2024 1:21 PM, Vladimir Oltean wrote:
>> On Wed, Aug 21, 2024 at 12:12:00PM -0700, Jacob Keller wrote:
>>> Ok. I'll investigate this, and I will send the two fixes for lib/packing
>>> in my series to implement the support in ice. That would help on our end
>>> with managing the changes since it avoids an interdependence between
>>> multiple series in flight.
>>
>> There's one patch in there which replaces the packing(PACK) call with a
>> dedicated pack() function, and packing(UNPACK) with unpack(). The idea
>> being that it helps with const correctness. I still have some mixed
>> feelings about this, because a multiplexed packing() call is in some
>> ways more flexible, but apparently others felt bad enough about the
>> packing() API to tell me about it, and that stuck with me.
>>
>> I'm mentioning it because if you're going to use the API, you could at
>> least consider using the const-correct form, so that there's one less
>> driver to refactor later.
> 
> Yep! I've got those patches in my series now. Though I should note that
> I did not include any of the patches for the other drivers. I'll CC you
> when I send the series out, though it may likely go through our
> Intel-Wired-LAN tree first.
> 
> I've refactored your self tests into KUnit tests as well!
> 

I was writing additional tests and I think I ran into another issue with
QUIRK_MSB_ON_THE_RIGHT, when the bit offsets are not aligned to a byte
boundary:

When trying to unpack 0x1122334455667788 from the buffer between offsets
106-43, the calculation appears to completely break.

When packing:

> [18:34:50] box_bit_width = 3
> [18:34:50] box_start_bit = 2
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 2
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 5
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 3
> [18:34:50] new_box_start_bit = 1
> [18:34:50] new_box_end_bit = -3
> [18:34:50]     # packing_test_unpack: EXPECTATION FAILED at lib/packing_test.c:264
> [18:34:50]     Expected uval == params->uval, but
> [18:34:50]         uval == 1234605616436508544 (0x1122334455667780)
> [18:34:50]         params->uval == 1234605616436508552 (0x1122334455667788)
> [18:34:50] [FAILED] msb right, 16 bytes, non-aligned
> [18:34:50] # packing_test_unpack: pass:19 fail:1 skip:0 total:20

Notice that the box end bit is now negative. Specifically this is
because the width is smaller than the start bit, so subtraction underflows.

When unpacking:
> [18:34:50] box_bit_width = 3
> [18:34:50] box_start_bit = 2
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 2
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 8
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 0
> [18:34:50] new_box_start_bit = 7
> [18:34:50] new_box_end_bit = 0
> [18:34:50] box_bit_width = 5
> [18:34:50] box_start_bit = 7
> [18:34:50] box_end_bit = 3
> [18:34:50] new_box_start_bit = 1
> [18:34:50] new_box_end_bit = -3
> [18:34:50]     # packing_test_unpack: EXPECTATION FAILED at lib/packing_test.c:264
> [18:34:50]     Expected uval == params->uval, but
> [18:34:50]         uval == 1234605616436508544 (0x1122334455667780)
> [18:34:50]         params->uval == 1234605616436508552 (0x1122334455667788)
> [18:34:50] [FAILED] msb right, 16 bytes, non-aligned
> [18:34:50] # packing_test_unpack: pass:19 fail:1 skip:0 total:20

Specifically, it looks like we basically fail to calculate valid new box
offsets.

What's weird to me is that when the box width is larger than the start
bit position, we just calculate the same exact offsets, so I don't see
why the existing calculations are there at all. Something is obviously
wrong here.