linux-kernel - Re: [PATCH 1/2] fs: configfs: make qw

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]

Message-ID: <7ca2aab805111faca014b0faea0965df@agner.ch>
Date:   Fri, 10 Feb 2017 12:29:53 -0800
From:   Stefan Agner <stefan@...er.ch>
To:     Felipe Balbi <balbi@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>, andrzej.p@...sung.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

On 2017-02-10 04:30, Felipe Balbi wrote:
> Hi,
> 
> Greg KH <gregkh@...uxfoundation.org> writes:
>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>>> On 2017-02-01 08:59, Stefan Agner wrote:
>>> > On 2017-02-01 00:06, Greg KH wrote:
>>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> >>> when read. This isn't obvious when simply using cat since the null
>>> >>> characters are not visible, but hexdump unveils the true string:
>>> >>>
>>> >>>   # echo MSFT100 > os_desc/qw_sign
>>> >>>   # hexdump -C os_desc/qw_sign
>>> >>>   00000000  4d 00 53 00 46 00 54 00  31 00 30 00 30 00        |M.S.F.T.1.0.0.|
>>> >>>
>>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> >>> common convention and add a new line at the end.
>>> >>
>>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>>> >> convert them?
>>> >
>>> > That is a discussion we should have had when the write side of this has
>>> > been added:
>>> >
>>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>>> > char *page,
>>> > 				     size_t len)
>>> > {
>>> > 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> > 	int res, l;
>>> >
>>> > 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>>> > 	if (page[l - 1] == '\n')
>>> > 		--l;
>>> >
>>> > 	mutex_lock(&gi->lock);
>>> > 	res = utf8s_to_utf16s(page, l,
>>> > 			      UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>>> > 			      OS_STRING_QW_SIGN_LEN);
>>> > 	if (res > 0)
>>> > 		res = len;
>>> > 	mutex_unlock(&gi->lock);
>>> >
>>> > 	return res;
>>> > }
>>> >
>>> >
>>> > The store function is definitely already in use today, e.g. this script
>>> > used for ev3dev:
>>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>>> >
>>> > Changing it to UTF-16 would break that script... So changing the store
>>> > part is the lesser of two evils.
>>> >
>>> > Regarding new line: Just following what other attributes are doing by
>>> > using the GS_STRINGS_R macro.
>>>
>>> Any comment on this? In my opinion especially this first patch really
>>> fixes a bug and should get applied... I can remove the newline if
>>> preferred.
>>
>> It's up to Felipe, give him a chance to catch up on patches...
> 
> I really don't know what to do here :-) Either way have the potential of
> breaking userspace. Maybe returning UTF8 as in write is the lesser of
> two evils.

So far libusbg and libusbgx do not have support for OS Descriptors.

I came across the issue while writing support for libusbgx (not upstream
yet). And it is rather ugly to have to do it on the read side and not to
do on the write side. It would also extend the dependencies of the
library (I implemented a prototype using iconv).

Also reading the old interface with UTF-8 does not crash the library, it
just return only the first character (since the second character is
\0...).

--
Stefan