linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Larry Finger <Larry.Finger@lwfinger.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-staging@lists.linux.dev" <linux-staging@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()
Date: Fri, 23 Jul 2021 19:40:35 +0200	[thread overview]
Message-ID: <3650881.QlJdx9khu8@localhost.localdomain> (raw)
In-Reply-To: <f396ffee4a414ee092625ee486b871fe@AcuMS.aculab.com>

Hi David,

This driver is going to be replaced by a "better" version, so I'm not sure 
whether or not this patch is still needed.

However, I see that we have similar problems in rtl8723bs and perhaps also in 
other drivers. Therefore, I'd like to solve this problem, whatever will happen 
to the "worse" rti8188eu, and change the code where else it needs to be 
changed.

Now I have a few questions... 

On Thursday, July 22, 2021 5:30:08 PM CEST David Laight wrote:
> From: Fabio M. De Francesco
> 
> > Sent: 21 July 2021 12:01
> > 
> > Use crc32_le() in place of the custom getcrc32().
> 
> ...
> 
> > @@ -609,14 +595,15 @@ u32	rtw_tkip_encrypt(struct adapter *padapter, 
struct xmit_frame
> > *pxmitframe)
> > 
> >  				if ((curfragnum + 1) == pattrib-
>nr_frags) {	/* 4 the last fragment */
> >  				
> >  					length = pattrib-
>last_txcmdsz - pattrib->hdrlen - pattrib->iv_len -
> > 
> > pattrib->icv_len;
> > -					*((__le32 *)crc) = 
getcrc32(payload, length);/* modified by Amy*/
> > +					*((__le32 *)crc) = 
cpu_to_le32(~crc32_le(~0, payload, length));
> > 
> >  					
arcfour_init(&mycontext, rc4key, 16);
> >  					
arcfour_encrypt(&mycontext, payload, payload, length);
> >  					
arcfour_encrypt(&mycontext, payload + length, crc, 4);
> >  				
> >  				} else {
> >  				
> >  					length = pxmitpriv-
>frag_len - pattrib->hdrlen - pattrib->iv_len -
> > 
> > pattrib->icv_len;
> > -					*((__le32 *)crc) = 
getcrc32(payload, length);/* modified by Amy*/
> > +					*((__le32 *)crc) = 
cpu_to_le32(~crc32_le(~0, payload, length));
> > +
> > 
> >  					
arcfour_init(&mycontext, rc4key, 16);
> >  					
arcfour_encrypt(&mycontext, payload, payload, length);
> >  					
arcfour_encrypt(&mycontext, payload + length, crc, 4);
> 
> Change crc to be __le32, kill the casts and pass &crc in the last call.
> 

I could do it, but the last call (that to arcfour_encrypt() takes a pointer to 
u8 type as the third parameter. How can I use a __le32 for that?

I think that one possible solution is to change crc to be an union and use two 
fields in the following lines:

union {
	__le32 f0; /* More descriptive name? */
	u8 f1[4]; /* More descriptive name? */
} crc;

[...]

crc.f0 = cpu_to_le32(~crc32_le(~0, payload, length - 4));

[...]

arcfour_encrypt(&mycontext, payload + length, crc.f1, 4);

Please, tell me... What about the solution above?

> > @@ -682,7 +669,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, struct 
recv_frame
> > *precvframe)> 
> >  			arcfour_init(&mycontext, rc4key, 16);
> >  			arcfour_encrypt(&mycontext, payload, 
payload, length);
> > 
> > -			*((__le32 *)crc) = getcrc32(payload, length 
- 4);
> > +			*((__le32 *)crc) = cpu_to_le32(~crc32_le(~0, 
payload, length - 4));
> > 
> >  			if (crc[3] != payload[length - 1] ||
> >  			
> >  			    crc[2] != payload[length - 2] ||
> 
> You could to the same here, or make crc u32, remove the cpu_to_le32()
> and use get_unaligned_u32(payload + length - 4) (or whatever it is called).
> 

Sorry, I can't understand this line. Can you please elaborate it a bit more?

> But it is much better to do:
> 	crc = crc32_le(~0, payload, length);
> 	if (crc != VALID_CRC32)
> 		res = _FAIL;
>

Why "crc = crc32_le(~0, payload, length);"? Shouldn't it be "crc = 
cpu_to_le32(~crc32_le(~0, patload, length);"? 

Why did you drop both the cpu_to_le32() call and the '~' operator?

Thanks in advance,

Fabio

> You can lookup VALID_CRC32, but it is crc32_le(0, "\xff\xff\xff\xff", 4);
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
1PT, UK
> Registration No: 1397386 (Wales)





  reply	other threads:[~2021-07-23 17:40 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 11:00 [PATCH v3 0/2] " Fabio M. De Francesco
2021-07-21 11:00 ` [PATCH v3 1/2] " Fabio M. De Francesco
2021-07-22 15:30   ` David Laight
2021-07-23 17:40     ` Fabio M. De Francesco [this message]
2021-07-24 11:04       ` David Laight
2021-07-21 11:00 ` [PATCH v3 2/2] staging: rtl8188eu: Remove no more used functions and variables Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3650881.QlJdx9khu8@localhost.localdomain \
    --to=fmdefrancesco@gmail.com \
    --cc=David.Laight@aculab.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --subject='Re: [PATCH v3 1/2] staging: rtl8188eu: Replace a custom function with crc32_le()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox