| 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
| ||
|
Message-ID: <bbdabd48-61c0-46f9-bf33-c49d6d27ffb0@linux.dev>
Date: Tue, 26 Aug 2025 11:14:19 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Yibo Dong <dong100@...se.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, corbet@....net,
gur.stavi@...wei.com, maddy@...ux.ibm.com, mpe@...erman.id.au,
danishanwar@...com, lee@...ger.us, gongfan1@...wei.com, lorenzo@...nel.org,
geert+renesas@...der.be, Parthiban.Veerasooran@...rochip.com,
lukas.bulwahn@...hat.com, alexanderduyck@...com, richardcochran@...il.com,
kees@...nel.org, gustavoars@...nel.org, netdev@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
On 26/08/2025 02:31, Yibo Dong wrote:
> On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
>> On 22/08/2025 03:34, Dong Yibo wrote:
>>
>> [...]
>>> +/**
>>> + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
>>> + * @hw: pointer to the HW structure
>>> + * @req: pointer to the cmd req structure
>>> + * @cookie: pointer to the req cookie
>>> + *
>>> + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
>>> + * reply. cookie->wait will be set in irq handler.
>>> + *
>>> + * @return: 0 on success, negative on failure
>>> + **/
>>> +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
>>> + struct mbx_fw_cmd_req *req,
>>> + struct mbx_req_cookie *cookie)
>>> +{
>>> + int len = le16_to_cpu(req->datalen);
>>> + int err;
>>> +
>>> + cookie->errcode = 0;
>>> + cookie->done = 0;
>>> + init_waitqueue_head(&cookie->wait);
>>> + err = mutex_lock_interruptible(&hw->mbx.lock);
>>> + if (err)
>>> + return err;
>>> + err = mucse_write_mbx_pf(hw, (u32 *)req, len);
>>> + if (err)
>>> + goto out;
>>> + /* if write succeeds, we must wait for firmware response or
>>> + * timeout to avoid using the already freed cookie->wait
>>> + */
>>> + err = wait_event_timeout(cookie->wait,
>>> + cookie->done == 1,
>>> + cookie->timeout_jiffies);
>>
>> it's unclear to me, what part of the code is managing values of cookie
>> structure? I didn't get the reason why are you putting the address of
>> cookie structure into request which is then directly passed to the FW.
>> Is the FW supposed to change values in cookie?
>>
>
> cookie will be used in an irq-handler. like this:
> static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
> struct mbx_fw_cmd_reply *reply)
> {
> struct mbx_req_cookie *cookie;
>
> cookie = reply->cookie;
>
> if (cookie->priv_len > 0)
> memcpy(cookie->priv, reply->data, cookie->priv_len);
> cookie->done = 1;
> if (le16_to_cpu(reply->flags) & FLAGS_ERR)
> cookie->errcode = -EIO;
> else
> cookie->errcode = 0;
> wake_up(&cookie->wait);
> return 0;
> }
> That is why we must wait for firmware response.
> But irq is not added in this patch series. Maybe I should move all
> cookie relative codes to the patch will add irq?
well, yes, in general it's better to introduce the code as a solid
solution. this way it's much easier to review
>
>>> +
>>> + if (!err)
>>> + err = -ETIMEDOUT;
>>> + else
>>> + err = 0;
>>> + if (!err && cookie->errcode)
>>> + err = cookie->errcode;
>>> +out:
>>> + mutex_unlock(&hw->mbx.lock);
>>> + return err;
>>> +}
>>
>> [...]
>>
>>> +struct mbx_fw_cmd_req {
>>> + __le16 flags;
>>> + __le16 opcode;
>>> + __le16 datalen;
>>> + __le16 ret_value;
>>> + union {
>>> + struct {
>>> + __le32 cookie_lo;
>>> + __le32 cookie_hi;
>>> + };
>>> +
>>> + void *cookie;
>>> + };
>>> + __le32 reply_lo;
>>> + __le32 reply_hi;
>>
>> what do these 2 fields mean? are you going to provide reply's buffer
>> address directly to FW?
>>
>
> No, this is defined by fw. Some fw can access physical address.
> But I don't use it in this driver.
FW can access physical address without previously configuring IOMMU?
How can that be?
>
>>> + union {
>>> + u8 data[32];
>>> + struct {
>>> + __le32 version;
>>> + __le32 status;
>>> + } ifinsmod;
>>> + struct {
>>> + __le32 port_mask;
>>> + __le32 pfvf_num;
>>> + } get_mac_addr;
>>> + };
>>> +} __packed;
>>> +
>>> +struct mbx_fw_cmd_reply {
>>> + __le16 flags;
>>> + __le16 opcode;
>>> + __le16 error_code;
>>> + __le16 datalen;
>>> + union {
>>> + struct {
>>> + __le32 cookie_lo;
>>> + __le32 cookie_hi;
>>> + };
>>> + void *cookie;
>>> + };
>>
>> This part looks like the request, apart from datalen and error_code are
>> swapped in the header. And it actually means that the FW will put back
>> the address of provided cookie into reply, right? If yes, then it
>> doesn't look correct at all...
>>
>
> It is yes. cookie is used in irq handler as show above.
> Sorry, I didn't understand 'the not correct' point?
The example above showed that the irq handler uses some value received
from the device as a pointer to kernel memory. That's not safe, you
cannot be sure that provided value is valid pointer, and that it points
to previously allocated cookie structure. It is a clear way to corrupt
memory.
>
>>> + union {
>>> + u8 data[40];
>>> + struct mac_addr {
>>> + __le32 ports;
>>> + struct _addr {
>>> + /* for macaddr:01:02:03:04:05:06
>>> + * mac-hi=0x01020304 mac-lo=0x05060000
>>> + */
>>> + u8 mac[8];
>>> + } addrs[4];
>>> + } mac_addr;
>>> + struct hw_abilities hw_abilities;
>>> + };
>>> +} __packed;
>>
Powered by blists - more mailing lists