Re: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Hi Roland,

Le jeudi 22 mai 2014 à 08:21 +0200, Yann Droneaud a écrit :
> Le lundi 05 mai 2014 à 19:35 +0200, Yann Droneaud a écrit :
> > i386 ABI disagree with most other ABIs regarding alignment
> > of data type larger than 4 bytes: on most ABIs a padding must
> > be added at end of the structures, while it is not required on
> > i386.
> > 
> > So for most ABI struct c4iw_alloc_ucontext_resp get implicitly padded
> > to be aligned on a 8 bytes multiple, while for i386, such padding
> > is not added.
> > 
> > Tool pahole could be used to find such implicit padding:
> > 
> >   $ pahole --anon_include \
> >            --nested_anon_include \
> >            --recursive \
> >            --class_name c4iw_alloc_ucontext_resp \
> >            drivers/infiniband/hw/cxgb4/iw_cxgb4.o
> > 
> > Then, structure layout can be compared between i386 and x86_64:
> > 
> >   +++ obj-i386/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt   2014-03-28 11:43:05.547432195 +0100
> >   --- obj-x86_64/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt 2014-03-28 10:55:10.990133017 +0100
> >   @@ -2,9 +2,8 @@ struct c4iw_alloc_ucontext_resp {
> >           __u64                      status_page_key;      /*     0     8 */
> >           __u32                      status_page_size;     /*     8     4 */
> > 
> >   -       /* size: 12, cachelines: 1, members: 2 */
> >   -       /* last cacheline: 12 bytes */
> >   +       /* size: 16, cachelines: 1, members: 2 */
> >   +       /* padding: 4 */
> >   +       /* last cacheline: 16 bytes */
> >    };
> > 
> > This ABI disagreement will make an x86_64 kernel try to write
> > past the buffer provided by an i386 binary.
> > 
> > When boundary check will be implemented, the x86_64 kernel will
> > refuse to write past the i386 userspace provided buffer
> > and the uverbs will fail.
> > 
> > If the structure lay in memory on a page boundary and next page
> > is not mapped, ib_copy_to_udata() will fail and the uverb
> > will fail.
> > 
> > Additionally, as reported by Dan Carpenter, without the implicit
> > padding being properly cleared, an information leak would take
> > place in most architectures.
> > 
> > This patch adds an explicit padding to struct c4iw_alloc_ucontext_resp,
> > and, like 92b0ca7cb149 ('IB/mlx5: Fix stack info leak in
> > mlx5_ib_alloc_ucontext()'), makes function c4iw_alloc_ucontext()
> > not writting this padding field to userspace. This way, x86_64 kernel
> > will be able to write struct c4iw_alloc_ucontext_resp as expected by
> > unpatched and patched i386 libcxgb4.
> > 
> > Link: http://marc.info/?i=cover.1399309513.git.ydroneaud@opteya.com
> > Link: http://marc.info/?i=1395848977.3297.15.camel-bi+AKbBUZKaGOdeMZTrFbA@public.gmane.orgldomain
> > Link: http://marc.info/?i=20140328082428.GH25192@mwanda
> > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')
> > Reported-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> > Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> 
> I believe this one should go in v3.15-rc7 as it fixes an issue
> introduced in v3.15-rc1. See 
> http://marc.info/?i=20140328082428.GH25192@mwanda
> http://marc.info/?i=20140502235616.GJ4963@mwanda
> 
> The other patchs could probably wait for v3.16-rc1 for integration in
> linux-stable.
> 

I think this patch is likely not going to v3.15, so in order to have it 
integrated in v3.15.x with the others, could you add Cc: 
<stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> to  this patch the next time you rebuild
your 'for-next' branch ?

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

Thanks a lot.

Regards.

-- 
Yann Droneaud
OPTEYA


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