Re: [PATCH v3] rtlwifi: rtl8192se firmware load can overflow targetbuffer

[Larry Finger]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/