linux-kernel - Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 Sep 2012 09:53:06 -0700
From:	Kevin Daughtridge <kevin@...u.com>
To:	Jiri Slaby <jslaby@...e.cz>, Jiri Kosina <jkosina@...e.cz>
CC:	Henrik Rydberg <rydberg@...omail.se>,
	Kevin Daughtridge <kevin@...u.com>,
	linux-input@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

On 09/19/12 01:05 N.U., Jiri Slaby wrote:
> AFAICS this is incorrect. Some drivers return pointers to their own 
> static structure from their .report_fixup. Hence there are two problems:
> * leak, because kmemdup'ped start is never freed
> * invalid free -- kfree(device->rdesc) will try to free a static 
> structure regards, 

On 09/19/12 04:55 N.U., Jiri Kosina wrote:
> How do you avoid memory leak on 'start' here? 

Hmm. I hadn't noticed that the other drivers are returning a static 
structure. In that case, it seems that report_fixup itself is broken 
from a memory perspective, in that it returns pointers to inconsistent 
storage types depending on the driver. (As evidenced by how my first 
patch version would have also caused invalid frees that weren't evident 
from the declaration in hid.h.) I see two options:

1. Ugly workaround: make a temporary copy of the dev_rdesc, give it to 
report_fixup, make a copy of the return, store that copy in rdesc, free 
the temporary copy. Though ugly, this would at least involve the 
smallest diff.

2. Standardize the behavior of the drivers' report_fixup 
implementations. Given that some of them need to change the size of the 
descriptor, modifying the passed structure is not an option. Probably 
all of them should return a newly allocated structure, either a modified 
copy of the input or a copy of their static, that can then be stored 
directly in rdesc. Especially since report_fixup is only ever called 
right before a copy is going to be taken anyway. (Adding constness to a 
parameter isn't considered a severe ABI break, is it?)

Thoughts?

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