On Fri, 01 Jul 2016 11:08:23 +0200, Takashi Sakamoto wrote:
On Jul 1 2016 17:50, Takashi Iwai wrote:
On Fri, 01 Jul 2016 10:30:20 +0200, Takashi Sakamoto wrote:
On Jul 1 2016 16:19, Takashi Iwai wrote:
On Fri, 01 Jul 2016 06:15:10 +0200, Takashi Sakamoto wrote:
The 'dimen' field in struct snd_ctl_elem_info is used to compose all of members in the element as multi-dimensional matrix. The field has four members. Each member represents the width in each dimension level by element member unit. For example, if the members consist of typical two dimensional matrix, the dimen[0] represents the number of rows and dimen[1] represents the number of columns (or vise-versa).
The total members in the matrix should be within the number of members in the element, while current implementation has no validator of this information. In a view of userspace applications, the information must be valid so that it cannot cause any bugs such as buffer-over-run.
This commit adds a validator of dimension information for userspace applications which add new element sets. When they add the element sets with wrong dimension information, they receive -EINVAL.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/sound/core/control.c b/sound/core/control.c index a85d455..eeb080f 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -805,6 +805,43 @@ static int snd_ctl_elem_list(struct snd_card *card, return 0; }
+static bool validate_element_member_dimension(struct snd_ctl_elem_info *info) +{
- unsigned int members;
- unsigned int i = 0;
- /* The value for each level should be zero or positive. */
- if (info->dimen.d[0] < 0)
return false;- members = info->dimen.d[0];
- if (members > 0) {
for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {if (info->dimen.d[i] < 0)return false;if (info->dimen.d[i] == 0)break;/* Prevention of division by zero, for safe. */if (members == 0)return false;/* Prevent arithmetic overflow. */if (info->dimen.d[i] > UINT_MAX / members)return false;members *= info->dimen.d[i];}- }
- /* The rest of level should be zero. */
- for (++i; i < ARRAY_SIZE(info->dimen.d); ++i) {
if (info->dimen.d[i] != 0)return false;- }
- return members <= info->count;
Well, since the dimen is 16bit short, it's easier to check the overflow just by comparing the value with info->count at each time (supposing info->count being the right value).
Your logic still causes arithmetic overflow. Let's assume a case that the first element of dimension info has 3 or more and the second has SHRT_MAX.
Remember that elements is 32bit unsigned int...
Oops. A document in my hand has wrong value about it...
Anyway, it's better to avoid evaluation after calculation because we can assume that SHRT_MAX x SHRT_MAX from raw dimension info.
SHRT_MAX * SHRT_MAX can't overflow. You can compare the result with info->count safely. Take a look at the code closely. It's comparison *in the loop* at each multiplication. This is how to check the overflow more easily. (And it's a kind of standard procedure.)
It's better to avoid evaluation after calculation.
That is, something like this:
/* If no dimension is given, it's OK */ if (!info->dimen.d[0]) return true;
elements = 1; for (i = 0; i < ARRAY_SIZE(info->dimen.d); i++) { if (info->dimen.d[i] < 0) return false; if (!info->dimen.d[i]) break;
Were I you, I would insert codes to evaluate the element of dimension info; i.e.
if (info->dimen.d[i] > 512) break;Here, 512 is the maximum number of members which an element can have. In this case, it's certainly an element of byte type.
It's superfluous. If info->count is already a sane value, it'd be enough to compare with this.
The info->count comes from userspace or each driver. It's dangerous to use it for avoiding arithmetic overflow.
Your function is to verify the dimen array. And for that, a sane info->count value is prerequisite. Otherwise how can you validate it at all...?
Takashi