netdev - Re: [PATCH v3] rtlwifi: rtl8192se firmware load can overflow target buffer

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] [day] [month] [year] [list]

Message-ID: <4F3462F0.20905@lwfinger.net>
Date:	Thu, 09 Feb 2012 18:21:04 -0600
From:	Larry Finger <Larry.Finger@...inger.net>
To:	tim.gardner@...onical.com
CC:	Tim Gardner <rtg.canonical@...il.com>,
	Ben Hutchings <bhutchings@...arflare.com>,
	Chaoming Li <chaoming_li@...lsil.com.cn>,
	"John W. Linville" <linville@...driver.com>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rtlwifi: rtl8192se firmware load can overflow target
 buffer

On 02/09/2012 11:25 AM, Tim Gardner wrote:
> On 02/09/2012 09:43 AM, Larry Finger wrote:
>> On 02/09/2012 08:41 AM, Tim Gardner wrote:
>>>
>>> I agree with you about the semantics of rtlpriv->max_fw_size, but I
>>> don't agree
>>> that the size check is correct. While rtlpriv->max_fw_size has been
>>> set to
>>> sizeof(struct rt_firmware), that value is _way_ bigger then the size
>>> of the
>>> target buffer.
>>>
>>> sizeof(struct rt_firmware) == 64000+64000+164000 plus some change
>>>
>>> The target buffer size is only 164000 bytes.
>>>
>>> I've attached v2 of the patch that is simpler and may serve to better
>>> illustrate
>>> my point. By the way, Ben Hutchings was right about the original patch
>>> having an
>>> off by one error. This version also clears rtlpriv->max_fw_size if the
>>> size
>>> check fails. Probably should have mentioned that in the commit log.
>>
>> I agree that Ben is right.
>>
>> This thread forced me to go back to square one in analyzing the
>> situation. For the other drivers in the rtlwifi family, the firmware
>> file contains an image that is directly stuffed into the device. For the
>> RTL8192SE devices, it is more complicated. The structure is described in
>> struct rt_firmware. At the moment, the arrays there are grossly
>> oversized. They could be as follows:
>>
>> struct rt_firmware {
>> struct fw_hdr *pfwheader;
>> enum fw_status fwstatus;
>> u16 firmwareversion;
>> u8 fw_imem[RTL8190_MAX_IMEM_CODE_SIZE];
>> u8 fw_emem[RTL8190_MAX_DMEM_CODE_SIZE];
>> u32 fw_imem_len;
>> u32 fw_emem_len;
>> u8 sz_fw_tmpbuffer[RTL8190_MAX_FIRMWARE_SIZE];
>> u32 sz_fw_tmpbufferlen;
>> u16 cmdpacket_fragthresold;
>> };
>>
>> with
>>
>> RTL8190_MAX_IMEM_CODE_SIZE = 54000 (current fw is 51,208),
>> RTL8190_MAX_DMEM_CODE_SIZE = 40000 (current fw is 37,520), and
>> RTL8190_MAX_FIRMWARE_SIZE = 90000 (it holds the raw firmware image,
>> which is currently 88,856).
>>
>> Ultimately, all three arrays should be eliminated. Now that we are using
>> asynchronous loading, the kernel should keep its cached data and not
>> copy it into the driver's private storage when a pointer will suffice.
>> All the drivers need this change, but that can wait for now.
>>
>> I will ACK the patch if you resumit it with
>> #define RTL8190_MAX_RAW_FIRMWARE_CODE_SIZE 90000
>>
>> Larry
>
> v3 expands the commit log a bit. It doesn't apply to stable 3.2.y, but could
> easily be backported. If I remember I'll do it when its merged in Linus' tree.

I added my S-O-B and pushed it on to John.

Thanks,

Larry

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html