linux-kernel - Re: [PATCH v3 03/33] staging: rtl8192e: Mark unaligned memcpy()

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]

Date:	Mon, 18 May 2015 12:09:18 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Mateusz Kulikowski <mateusz.kulikowski@...il.com>
Cc:	devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 03/33] staging: rtl8192e: Mark unaligned memcpy()

On Sun, May 17, 2015 at 10:38:48PM +0200, Mateusz Kulikowski wrote:
> On 15.05.2015 01:14, Dan Carpenter wrote:
> > On Thu, May 14, 2015 at 10:29:39PM +0200, Mateusz Kulikowski wrote:
> >> On 13.05.2015 10:22, Dan Carpenter wrote:
> >>> On Tue, May 12, 2015 at 10:00:13PM +0200, Mateusz Kulikowski wrote:
> >>>> On 11.05.2015 10:26, Dan Carpenter wrote:
> >> (...)
> >>>>>
> >>>>> Which part isn't aligned?  I think they both are.
> >>>>>
> >>>> struct rtllib_rxb *prxb = prxbIndicateArray[j];
> >>>>
> >>>> struct rtllib_rxb {
> >>>> 	u8 nr_subframes;
> >>>> 	struct sk_buff *subframes[MAX_SUBFRAME_COUNT == 64];
> >>>> 	u8 dst[ETH_ALEN]; // here
> >>>> 	u8 src[ETH_ALEN]; // here
> >>>> } __packed;
> (...)
> > 
> > I'm not positive it's "by design" though, this is staging code so maybe
> > they just added __packed to every struct.  In fact, I'm pretty sure
> > unaligned pointers don't work on some arches so the __packed is probably
> > a bug.
> > 
> 
> I doubt anyone uses it on anything else than some low cost x86 netbooks. 
> 

Generally though, there shouldn't be pointers in __packed structs.
__packed means we care about alignment very much, probably more than we
care about speed.  So it means we are sharing the data with the hardware
or with userspace which requires a specific layout.  We shouldn't be
giving kernel pointers to userspace or the hardware.  (These are rules
of thumb).

> I removed __packed and did a small test today (download 10mb of garbage, check md5)
> - it doesn't seem to affect driver (on staging-testing, without this patchset).
> 
> Nevertheless I would prefer to leave it like that for this patch set if it's
> OK with you and perhaps include it for next series I'll work on.

I think Greg asked for a v4 of this patchset?  You can leave the code
as is if you want.  :)  You didn't introduce the bug and I'm not your
boss.  :P

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/