linux-kernel - Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

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: <d253f864-5585-ce29-b9b4-92a0e2bc374@linux.intel.com>
Date:   Tue, 25 Oct 2022 14:50:14 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Starke, Daniel" <daniel.starke@...mens.com>
cc:     linux-serial <linux-serial@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Tue, 25 Oct 2022, Starke, Daniel wrote:

> > > +	n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
> > 
> > Should this be using get_unaligned...()?
> 
> Is this really necessary if the structure is already __packed? I did not
> receive any warning by the compiler.

It would be arch dependent to begin with. But honestly, I'm not entirely 
certain here myself.

Documentation/core-api/unaligned-memory-access.rst claims compiler would 
indeed do extra work to ensure access of unaligned member in a packed 
struct is handled ok. But then you call le16_to_cpu() for the member field 
which is full of cast magic so I'd be a bit hesitant to claim the 
knowledge about the unalignment is carried all the way down there through 
those casts.

Other subtle detail is the reply side struct which is allocated from stack
and with packed compiler is allowed (I don't know if it does that or not)
to make the struct unaligned as well (so perhaps put_unaligned would be 
necessary there too if packed is retained).

If you want my recommendation, I'd just remove the packed altogether from 
the struct because there seems to be no natural holes in it, use 
get_unaligned for the receive side, and add this build time check:

static_assert(sizeof(struct gsm_dlci_param_bits) == 8);

If lkp builds all its current archs fine with that static_assert(), I'd be 
pretty sure the struct that the unpacked struct is ok on all archs. Would 
it ever stop being true on any arch/compiler setting, the assert would 
catch it right away.

-- 
 i.