Messages in this thread | | | Date | Fri, 11 Dec 2015 00:17:20 +0000 | From | One Thousand Gnomes <> | Subject | Re: [PATCH v2] staging: gdm72xx: add userspace data struct |
| |
On Fri, 11 Dec 2015 00:47:38 +0100 Wim de With <nauxuron@wimdewith.com> wrote:
> On Thu, Dec 10, 2015 at 02:44:45PM +0000, One Thousand Gnomes wrote: > > (except that you mean sizeof(struct fsm_s) and it doesn't compile at the > > moment! > > Oops, sloppy mistake.
Compile/test/send - even when in a hurry
> > data_s can just be modified to be __user. All uses of it follow that > > rule. > > What do you mean? The data still needs to be copied from user space to kernel > space, if I'm not mistaken. And not all uses follow that rule, since in both > gdm_wimax_ioctl_get_data() and gdm_wimax_ioctl_set_data() it is used as both > the source and destination in the copy_from_user() and copy_to_user() call.
Good point I missed that.
> > All I think you need in this case is > > > > struct fsm_s fsm_buf; > > > > if (copy_from_user(&fsm_buf, req->data.buf,sizeof(buf)) > > return -EFAULT > > gdm_update_fsm(&fsm_buf); > > Do you mean sizeof(fsm_s)? I realize this would have been far simpler than my > overkill solution.
Yes either sizeof(struct fsm_s) or sizeof(fsm_buf). The former is often safer.
> > If you are touching the structs it might be wise to fix the other > > problems with them notably the use of int. sizes when used are unsigned - > > and signed sizes are asking for errors. In fact if you look at the > > existing uses of the size checks they look deeply suspicious the moment > > anything malicious passes in negative numbers. > > I would love to do that, but it is a bit outside the scope of this patch, so I > would rather safe this for another patch.
Absolutely right - it should be another patch
Alan
| |