[benet] possible endianness bug in be_cmd_txq_create()

[benet] possible endianness bug in be_cmd_txq_create()

[Al Viro]

In be_cmd_txq_create() we have
        if (req->hdr.version > 0)
                req->if_id = cpu_to_le16(adapter->if_handle);
        req->num_pages = PAGES_4K_SPANNED(q_mem->va, q_mem->size);
        req->ulp_num = BE_ULP1_NUM;
        req->type = BE_ETH_TX_RING_TYPE_STANDARD;
        req->cq_id = cpu_to_le16(cq->id);
        req->queue_size = be_encoded_q_len(txq->len);
        be_cmd_page_addrs_prepare(req->pages, ARRAY_SIZE(req->pages), q_mem);
        ver = req->hdr.version;

req points to 

struct be_cmd_req_eth_tx_create {
        struct be_cmd_req_hdr hdr;
        u8 num_pages;
        u8 ulp_num;
        u16 type;
        u16 if_id;
        u8 queue_size;
        u8 rsvd0;
        u32 rsvd1;
        u16 cq_id;
        u16 rsvd2;
        u32 rsvd3[13];
        struct phys_addr pages[8];
} __packed;

Everything appears to be consistent with little-endian data - direct
assignments to u8 fields, cpu_to_le16 for cq_id and if_id, phys_addr
array is also filled with little-endian data, so's ->hdr (several
lines prior, by be_wrb_cmd_hdr_prepare()).

The only exception is
        req->type = BE_ETH_TX_RING_TYPE_STANDARD;
where we set a 16bit field with host-endian constant (2).

benet is playing silly buggers with swap-in-place in some places, but
it's always 32bit values getting swapped, so this can't be happening
here (num_pages, ulp_num and type form a 32bit-aligned word, and
on big-endian cpu_to_le32() done to it would've ended up with num_pages = 2,
ulp_num = 0, type = 256 + PAGES_4K_SPANNED(q_mem->va, q_mem->size), which is
unlikely to do anything good).

So it really smells like this line should've been
        req->type = cpu_to_le16(BE_ETH_TX_RING_TYPE_STANDARD);

I don't have the hardware, so the above is completely untested (caught by
sparse when trying to do endianness annotations in drivers/net), but it
does look like it might be worth a look from benet maintainers.

Re: [benet] possible endianness bug in be_cmd_txq_create()

[Al Viro]

On Sun, Dec 10, 2017 at 04:41:20PM +0000, Al Viro wrote:

> I don't have the hardware, so the above is completely untested (caught by
> sparse when trying to do endianness annotations in drivers/net), but it
> does look like it might be worth a look from benet maintainers.

Another very fishy place is be_roce_mcc_cmd().

        req = embedded_payload(wrb);
        resp = embedded_payload(wrb);

        be_wrb_cmd_hdr_prepare(req, hdr->subsystem,
                               hdr->opcode, wrb_payload_size, wrb, NULL);

OK, we'd formed a (little-endian) header in the first 32 bytes of req and
filled wrb->{tag0,tag1,payload_length,embedded}.

        memcpy(req, wrb_payload, wrb_payload_size);
        be_dws_cpu_to_le(req, wrb_payload_size);

... only to overwrite *req with wrb_payload bulk-converted to little-endian.
OK, so it's responsibility of caller to have prepare payload so that it
would form a valid header after byteswap.

        status = be_mcc_notify_wait(adapter);
        if (cmd_status)
                *cmd_status = (status & 0xffff);
        if (ext_status)
                *ext_status = 0;

OK, we submit the resulting wrb and wait for reply.

        memcpy(wrb_payload, resp, sizeof(*resp) + resp->response_length);
        be_dws_le_to_cpu(wrb_payload, sizeof(*resp) + resp->response_length);

Then we copy the response back into caller-supplied buffer, bulk-converting
it from little-endian to host-endian.  Hardware puts little-endian there,
caller wants host-endian.  But... ->response_length is a 32bit value at
offset 8 from the beginning of response.  Is it little-endian or host-endian?
In the former case we are fucked on b-e host - the value we pass to memcpy()
is going to be in hundreds of megabytes...  And everything in response seems
to be little-endian - we certainly include response_length itself into the
area covered by that bulk byteswap.

Re: [benet] possible endianness bug in be_cmd_txq_create()

[Al Viro]

On Sun, Dec 10, 2017 at 06:43:08PM +0000, Al Viro wrote:
> On Sun, Dec 10, 2017 at 04:41:20PM +0000, Al Viro wrote:
> 
> > I don't have the hardware, so the above is completely untested (caught by
> > sparse when trying to do endianness annotations in drivers/net), but it
> > does look like it might be worth a look from benet maintainers.
> 
> Another very fishy place is be_roce_mcc_cmd().

be_get_fw_log_level()/be_set_fw_log_level() look slightly fishy as well:
the latter has
                        if (cfgs->module[i].trace_lvl[j].mode == MODE_UART)
                                cfgs->module[i].trace_lvl[j].dbg_lvl =
                                                        cpu_to_le32(level);
while the former
                        if (cfgs->module[0].trace_lvl[j].mode == MODE_UART)
                                level = cfgs->module[0].trace_lvl[j].dbg_lvl;
and returns level without further conversions.  The caller of be_cmd_set_...()
pass a host-endian value as level (48 or 64); the caller of be_cmd_get_...()
expect a host-endian return value - it compares the result with 48.

There's almost certainly a missing conversion somewhere; at a guess -
le32_to_cpu() on the be_cmd_get_...() side.

Re: [benet] possible endianness bug in be_cmd_txq_create()

[Sathya Perla]

On Sun, Dec 10, 2017 at 10:11 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> In be_cmd_txq_create() we have
>         if (req->hdr.version > 0)
>                 req->if_id = cpu_to_le16(adapter->if_handle);
>         req->num_pages = PAGES_4K_SPANNED(q_mem->va, q_mem->size);
>         req->ulp_num = BE_ULP1_NUM;
>         req->type = BE_ETH_TX_RING_TYPE_STANDARD;
>         req->cq_id = cpu_to_le16(cq->id);
>         req->queue_size = be_encoded_q_len(txq->len);
>         be_cmd_page_addrs_prepare(req->pages, ARRAY_SIZE(req->pages), q_mem);
>         ver = req->hdr.version;
>
...
>
> Everything appears to be consistent with little-endian data - direct
> assignments to u8 fields, cpu_to_le16 for cq_id and if_id, phys_addr
> array is also filled with little-endian data, so's ->hdr (several
> lines prior, by be_wrb_cmd_hdr_prepare()).
>
> The only exception is
>         req->type = BE_ETH_TX_RING_TYPE_STANDARD;
> where we set a 16bit field with host-endian constant (2).
>
..
>
> So it really smells like this line should've been
>         req->type = cpu_to_le16(BE_ETH_TX_RING_TYPE_STANDARD);
>
Sorry for replying late...I missed this mail. I guess the old
@emulex.com email ids don't work anymore -- it's @broadcom.com now!
Yes, this does look like a bug; we'll send a patch to fix this.

Thanks!