linux-kernel - RE: [PATCH v2 7/7] udf: Merge linux specific translation into CS0 conversion function

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: <000101d14c74$6c1a5f40$444f1dc0$@mentor.com>
Date:	Mon, 11 Jan 2016 16:31:47 +0300
From:	Andrew Gabbasov <andrew_gabbasov@...tor.com>
To:	'Jan Kara' <jack@...e.cz>
CC:	Jan Kara <jack@...e.com>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 7/7] udf: Merge linux specific translation into CS0 conversion function

Hi Jan,

> -----Original Message-----
> From: Jan Kara [mailto:jack@...e.cz] 
> Sent: Monday, January 04, 2016 4:25 PM
> To: Gabbasov, Andrew
> Cc: Jan Kara; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2 7/7] udf: Merge linux specific translation into CS0
conversion function

[skipped]

> > +	u_ch = cmp_id >> 3;
> > +
> > +	ocu++;
> > +	ocu_len--;
> > +
>
> Can we just check whether ocu_len is a multiple of u_ch here
> and bail out if not? That would save us some rounding below
> and also fix possible access beyond the end of the string
> in case fs is corrupted.

Yes, I think it can be done. I just wasn't sure if having
unaligned length is allowed or not, and tried to support
this case too. We can definitely return -EINVAL if ocu_len
is not a multiple of u_ch.

[skipped]

> > -	for (i = 1; (i < ocu_len) && (str_o_len < str_max_len);) {
> >  		/* Expand OSTA compressed Unicode to Unicode */
> > -		uint32_t c = ocu[i++];
> > -		if (cmp_id == 16)
> > +		c = ocu[i++];
> > +		if (u_ch > 1)
> >  			c = (c << 8) | ocu[i++];
> >  
> > +		if (translate) {
> > +			if ((c == '.') && (firstDots >= 0))
> > +				firstDots++;
> > +			else
> > +				firstDots = -1;
> > +
> > +			if (c == '/' || c == 0) {
> > +				if (illChar)
> > +					continue;
> > +				illChar = 1;
> > +				needsCRC = 1;
> > +				c = ILLEGAL_CHAR_MARK;
> > +			} else {
> > +				illChar = 0;
> > +			}
> > +		}
> > +
> >  		len = conv_f(c, &str_o[str_o_len], str_max_len - str_o_len);
> >  		/* Valid character? */
> > -		if (len >= 0)
> > +		if (len >= 0) {
> >  			str_o_len += len;
> > -		else
> > +		} else {
> >  			str_o[str_o_len++] = '?';
> > +			needsCRC = 1;
> > +		}
>
> Umm, can you try factoring out body of this loop into a helper function
> and using it both during extension parsing and full name parsing?

I'll try to think about it. Although I'm not sure if it could become
a little overcomplicated since that helper could require too much internals
of this calling function.

> Also I think that checking whether the name is '.' or '..' could be moved
> just into the if (translate) below where you could just directly check it
like:
>	if (str_o_len <= 2 && str_o[0] == '.' &&
>	    (str_o_len == 1 || str_o[1] == '.'))

Yes, you are right, it will be simpler.
I'll do it (and get rid of firstDots) in version 3 of the patch.

I'll also make all your suggested changes with empty lines and += u_ch.

Thanks.

Best regards,
Andrew