linux-kernel - Re: [PATCH v3] Input: Add EVIOC mechanism for MT slots

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:	Sun, 5 Feb 2012 23:25:35 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Chase Douglas <chasedouglas@...il.com>
Cc:	Henrik Rydberg <rydberg@...omail.se>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Input: Add EVIOC mechanism for MT slots

On Sun, Feb 05, 2012 at 11:55:09PM +0100, Chase Douglas wrote:
> On 02/05/2012 08:40 PM, Henrik Rydberg wrote:
> >>> Besides leaving a possible giant stack crash in your code, it assumes
> >>> memory is somehow magically allocated. Not good practise in low-level
> >>> programming. You wouldn't use a template this way, would you?
> >>
> >> No, which is why I think this interface is bad. I should be able to
> >> dynamically set the size of the array, but it's not possible with this
> >> interface.
> > 
> > It is possible (using num_slots == 0 or creating your own struct), but
> > not ideal, granted. The patch serves the purpose of definining the
> > binary interface, the rest is up to userland.
> > 
> >> I think the implementation is fine in terms of how the plumbing works. I
> >> just don't think this macro should be included. Make the user create the
> >> struct themselves:
> >>
> >> In linux/input.h:
> >>
> >> struct input_mt_request {
> >> 	__u32 code;
> >> 	__s32 values[];
> >> };
> > 
> > The above (the first) version is not ideal either, since it cannot be
> > used as it is.
> > 
> >> It could be argued that we should leave the macro around for people who
> >> want to statically define the size of the request, but I think that is
> >> leading them down the wrong path. It's easier, but it will lead to
> >> broken code if you pick the wrong size.
> > 
> > Rather than creating both a suboptimal static and a suboptimal dynamic
> > version, removing the struct altogether is tempting. All that is
> > really needed is a clear definition of the binary interface. The rest
> > can easily be set up in userland, using whatever method is preferred.
> 
> I'm normally not a fan of static functions in header files, but maybe it
> would be a good solution here:
> 
> struct input_mt_request {
> 	__u32 code;
> 	__s32 values[];
> };
> 
> static struct input_mt_request *
> linux_input_mt_request_alloc(int num_slots) {
> 	return (struct input_mt_request *)malloc(
> 		sizeof(__u32) + num_slots * sizeof(__s32));
> }

Gah, can we please leave it to userspace programs to define and allocate
the structure as it makes most sense for them. As far as kernel goes we
just want to document that we'll need a u32 and a place to put N s32s.
How they are allocated we do not care at all. It could be on stack
(which does not have the same limits as kernel stack) or in the heap.

Thanks.

-- 
Dmitry
--
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/