Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570256 > unrolled thread

Re: [PATCH 60/60] staging: lustre: libcfs: fix minimum size check for libcfs ioctl

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 60/60] staging: lustre: libcfs: fix minimum size check  for libcfs ioctl James Simmons <jsimmons@infradead.org> - 2017-01-31 03:30 +0100
    Re: [PATCH 60/60] staging: lustre: libcfs: fix minimum size check  for libcfs ioctl Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-31 09:20 +0100
    Re: [lustre-devel] [PATCH 60/60] staging: lustre: libcfs: fix minimum  size check for libcfs ioctl Olaf Weber <olaf@sgi.com> - 2017-02-01 14:50 +0100
      Re: [lustre-devel] [PATCH 60/60] staging: lustre: libcfs: fix  minimum size check for libcfs ioctl Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-01 17:40 +0100

#1570256 — Re: [PATCH 60/60] staging: lustre: libcfs: fix minimum size check for libcfs ioctl

> It looks like what happened is there were two patches applied out of
> sync.  Let's add a fixes tag and CC the original author.

So the only problem here is the commit message. I will update it then.
 
> Fixes: ed2f549dc0f6 ("staging: lustre: libcfs: test if userland data is to small")
> 
> This patch was probably correct when it was written but commit
> 1290932728e5 ("staging: lustre: Dynamic LNet Configuration (DLC) IOCTL
> changes") ended up getting applied first so the size was wrong.
> 
> The lstcon_ioctl_entry() function doesn't have enough size checking.

This sounds like a separate patch. I will open a ticket about this and
your comments below.

> Also I'm uncomfortable with:
> 
> 	data = container_of(hdr, struct libcfs_ioctl_data, ioc_hdr);
> 
> If hdr isn't the first member of the struct then the code is broken but
> container_of() implies that that isn't a hard requirement.  It should
> just be:
> 
> 	data = (struct libcfs_ioctl_data *)hdr;

Don't know if hdr being first is a hard requirment. Doug, Amir do you know 
if it is an requirement? 

[toc] | [next] | [standalone]

#1570474

On Tue, Jan 31, 2017 at 02:25:22AM +0000, James Simmons wrote:
> This sounds like a separate patch. I will open a ticket about this and
> your comments below.

There are a some other places that need a size requirement like
LNetCtl().

It really feels like it should be a part of this patch because this
patch is introducing a security breakage and it's just fixing a normal
bug.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1571573 — Re: [lustre-devel] [PATCH 60/60] staging: lustre: libcfs: fix minimum size check for libcfs ioctl

On 31-01-17 03:25, James Simmons wrote:

[...]

>> Also I'm uncomfortable with:
>>
>> 	data = container_of(hdr, struct libcfs_ioctl_data, ioc_hdr);
>>
>> If hdr isn't the first member of the struct then the code is broken but
>> container_of() implies that that isn't a hard requirement.  It should
>> just be:
>>
>> 	data = (struct libcfs_ioctl_data *)hdr;
>
> Don't know if hdr being first is a hard requirment. Doug, Amir do you know
> if it is an requirement?

It's a requirement.

-- 
Olaf Weber                 SGI               Phone:  +31(0)30-6696796
                            Veldzigt 2b       Fax:    +31(0)30-6696799
Sr Software Engineer       3454 PW de Meern  Vnet:   955-6796
Storage Software           The Netherlands   Email:  olaf@sgi.com

[toc] | [prev] | [next] | [standalone]

#1571718 — Re: [lustre-devel] [PATCH 60/60] staging: lustre: libcfs: fix minimum size check for libcfs ioctl

On Wed, Feb 01, 2017 at 02:32:13PM +0100, Olaf Weber wrote:
> On 31-01-17 03:25, James Simmons wrote:
> 
> [...]
> 
> > > Also I'm uncomfortable with:
> > > 
> > > 	data = container_of(hdr, struct libcfs_ioctl_data, ioc_hdr);
> > > 
> > > If hdr isn't the first member of the struct then the code is broken but
> > > container_of() implies that that isn't a hard requirement.  It should
> > > just be:
> > > 
> > > 	data = (struct libcfs_ioctl_data *)hdr;
> > 
> > Don't know if hdr being first is a hard requirment. Doug, Amir do you know
> > if it is an requirement?
> 
> It's a requirement.

That's horrid.  Use container_of to be "safe" here please...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web