On Fri, 2021-02-19 at 05:15 +0000, Srinivasan Raju wrote:
> Hi,
>
> Please find a few responses to the comments , We will fix rest of the
> comments and submit v14 of the patch.
>
> > Also, you *really* need some validation here, rather than blindly
> > trusting that the file is well-formed, otherwise you immediately have a
> > security issue here.
>
> The firmware is signed and the harware validates the signature , so we are
> not validating in the software.
That wasn't my point. My point was that the kernel code trusts the
validity of the firmware image, in the sense of e.g. this piece:
> + no_of_files = *(u32 *)&fw_packed->data[0];
If the firmware file was corrupted (intentionally/maliciously or not), this
could now be say 0xffffffff.
> + for (step = 0; step < no_of_files; step++) {
> + start_addr = *(u32 *)&fw_packed->data[4 + (step * 4)];
But you trust it completely and don't even check that "4 + (step * 4)"
fits into the length of the data!
That's what I meant. Don't allow this to crash the kernel.
> > > +static const struct plf_reg_alpha2_map reg_alpha2_map[] = {
> > > + { PLF_REGDOMAIN_FCC, "US" },
> > > + { PLF_REGDOMAIN_IC, "CA" },
> > > + { PLF_REGDOMAIN_ETSI, "DE" }, /* Generic ETSI, use most restrictive
> > > */
> > > + { PLF_REGDOMAIN_JAPAN, "JP" },
> > > + { PLF_REGDOMAIN_SPAIN, "ES" },
> > > + { PLF_REGDOMAIN_FRANCE, "FR" },
> > > +};
> > You actually have regulatory restrictions on this stuff?
>
> Currently, There are no regulatory restrictions applicable for LiFi ,
> regulatory_hint setting is only for wifi core
So why do you have PLF_REGDOMAIN_* things? What does that even mean
then?
> > OTOH, I can sort of see why/how you're reusing wifi functionality here,
> > very old versions of wifi even had an infrared PHY I think.
> >
> > Conceptually, it seems odd. Perhaps we should add a new band definition?
> >
> > And what I also asked above - how much of the rate stuff is completely
> > fake? Are you really doing CCK/OFDM in some (strange?) way?
>
> Yes, your understanding is correct, and we use OFDM.
> For now we will use the existing band definition.
OFDM over infrared, nice.
Still, I'm not convinced we should piggy-back this on 2.4 GHz.
NL80211_BAND_1THZ? ;-)
Ok, I don't know what wavelength you're using, of course...
But at least thinking about this, wouldn't we be better off if we define
NL80211_BAND_INFRARED or something? That way, at least we can tell that
it's not going to interoperate with real 2.4 GHz, etc.
OTOH, this is a bit of a slippery slow - what if somebody else designs
an *incompatible* infrared PHY? I guess this is not really standardised
at this point.
Not really sure about all this, but I guess we need to think about it
more.
What are your reasons for piggy-backing on 2.4 GHz? Just practical "it's
there and we don't care"?
johannes
