Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Azeem Shaikh <azeemshaikh38@gmail.com>
CC:     Kees Cook <keescook@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Jeff Layton <jlayton@kernel.org>,
        "linux-hardening@vger.kernel.org" <linux-hardening@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] NFSD: Replace all non-returning strlcpy with strscpy
Thread-Topic: [PATCH] NFSD: Replace all non-returning strlcpy with strscpy
Thread-Index: AQHZg4w4w644hNPMzEqt/IwppCpl6q9VJ/yAgAAdSICAAXBDAIAAAiYA
Date:   Fri, 12 May 2023 14:38:27 +0000
Message-ID: <076418CA-6400-4A92-8634-60F2934F985D@oracle.com>
References: <20230510220952.3507366-1-azeemshaikh38@gmail.com>
 <72239648-C807-4CDD-8DA7-18440C83384E@oracle.com>
 <202305110927.12508719D2@keescook>
 <CADmuW3UbHBWN0JzXMYX667hkXyWJY8_88K1sJfRnKNg7_u38=A@mail.gmail.com>
In-Reply-To: <CADmuW3UbHBWN0JzXMYX667hkXyWJY8_88K1sJfRnKNg7_u38=A@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SZECm7cr2d+velbfke4TBjN5KAKt62if/6lstEf0SKpFgtkEfgqVQVmej3+Y?=
 =?us-ascii?Q?C1GBh6Z3eNEkdpRgW7BYLpSxAsuXXC3bfwZOJY5qyBlNE1UqcXmGV+4wyIWP?=
 =?us-ascii?Q?FsciKpZSB4ys/fc3AOcpHI7Jy8qPdm/m5jDPymkbixu/wTRBI/4Ddm79FJgA?=
 =?us-ascii?Q?qEbCOGB2MOPu/z072jO1p9Fa/pa75GMErC8JSNvGrcmczEyorsYu0fiFJG0D?=
 =?us-ascii?Q?WNB69XgfUt9dhHufjpR0PJuVGhd9dV+bMK2cOEPhLsRJszFU/+AJ+xf88gL/?=
 =?us-ascii?Q?CpplxQ1F6ERKXJrGAkmCzLGHUTIRUWaHLnK+cCfVIDkBQVXtmCZTc9yaYIxz?=
 =?us-ascii?Q?56B1J3fqq8fa54RBojP02pViEdSqmroZY3q3HMmGjl8oLl+0P2gU4DBfMf43?=
 =?us-ascii?Q?4Pm5I3uCMUSAxjqzC72Zqn6QaC1CWLPfqhCUOsmMGRsz0s+/6uHJbPYyrX56?=
 =?us-ascii?Q?gwSvWTZTD//Xs9DE0KheZN5rC7zKDhdSTNMyL1O9HijJPZ7FPhwpZr14g/C+?=
 =?us-ascii?Q?A61vRckq08obZ7Zps9CrFYq9ozK351C0+yD3i+1z14PvHtmAUmrLCw3uyUcs?=
 =?us-ascii?Q?ac0S2haMMacwX4harlMnIx6JY2+w4zDNHfqWT6lSqkR3UHS3PdXAkVwG6gs8?=
 =?us-ascii?Q?wD/0avwb3VS6uQubEodtKnDMn1hHNa7OrO1id6j6BKit8Mv3kpGnm27IUQqS?=
 =?us-ascii?Q?KReA91QfayDktoogZZz9rO1MvDrroqNWTYwwlIa6O13ChYnigpsYSEePlQRP?=
 =?us-ascii?Q?PwfxETXofXgesG5c9pjhQ8t4uo6bgKjUQidLK+d3WGZoAnGGiuUqQl7urK0W?=
 =?us-ascii?Q?wNyCpqc637gEiwmHnODWGTcI0sTC41Wc4zvBa9eoLeVWEHL9L2eIUHsrstjJ?=
 =?us-ascii?Q?PUIpPBWVPhMAurPwzBaVppd5DIA4d7kKrSTnOWv8PSY2qjvo1ciYQfKp/E90?=
 =?us-ascii?Q?2yYTmfVDt4w2wOqB993RDMVBrPCuhGDN3sLoB4YQGTdjw2a6x3O9zzR1YZL8?=
 =?us-ascii?Q?3KzLUX9waoia8+sWDjku0b4TG/EFg5U+NeWuwkrzZ/aLc+0M2/P+qGmLemsu?=
 =?us-ascii?Q?cVOveUW/wmxt22kZAHysXe9Clkhn3l3+afWWJnXQoRzpYB4vdxl0pBslSH/G?=
 =?us-ascii?Q?+vwffLz3xdeiYziJsimEzExVYIItr8k9Xuouv8rcavfUGzJWsh75zVOUj5mU?=
 =?us-ascii?Q?tbzBhB65PU5un5IRY+njP+DJho78sQZT1EUW0bGBqheQ05vjoexzfvlTL6cj?=
 =?us-ascii?Q?uw2P1j3J0q6XNSY/azMXKQhWcsJEbmczMZMVU4uZbg/G0osgESeBlS9CfPx7?=
 =?us-ascii?Q?c/f/oLttn7z+NRwM/I4fBJ7tdLkCUIVoE0Szz/CS6mmkJrrXClVfuPu1a6QH?=
 =?us-ascii?Q?9P6/bOUFXeasumM0wisPiKfLEugz+qhzagPAd+fE+JlDdTezh6g3tmvRj1vW?=
 =?us-ascii?Q?sZDz5CZIUvDuBK9bw+vRbMgq+yjwzuVLwUTppMi4QjNHtinyBE5YZvefDyDB?=
 =?us-ascii?Q?bfWAysoeVIgX0N3zftz6zu0la4n+Kj0fPh0p83QxRc+OrEHCo2DI7cnmcFME?=
 =?us-ascii?Q?hgIG7/2shDg7aR17BGy9rnJ6a8VRF5IODailpRzSfq4vrXftHquu3XnuTgrD?=
 =?us-ascii?Q?qg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EEF4D2CA85D437479449DBBAEC043A80@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: N2hBSpvF1St/cqYOl4+61+GKTO9PlrDaxu7GHvbrAqxcDdaV/xQQpfS86PJr7pdopYnTrbPUxtR9tZO0WyHJ4QGNvE+XwKTrkhM9i4nfJ8X7SWfAjIASwC5Q27SYXK3VWxNOz6VLLSEzQC+irR61HEN1ioYJQhciPebnHZWVuXnoj2D9xMMYrPON70Z/RZtjbbDz2mVypyC8Zb4WuzrzhmPV9NOT2g/Vfm2cmCWDYBS8J3qOSNuna6P9dchCVcxcIRhbMIDTI114+oXzmRBUTJQf7Z4MtvjwEOMhxXcVyeBfpglUVjErhlS/RCnIsVsyu5jqu4jQ2olJZwqInga/YbV6IPUSwADm9GOwzZH3XLv70Horg3IBzQrT9pyrdnBg+CHCwL8AsYl3qS80XCUAc8ALmcmW3qCm7KKUSVMb9jdQSXkQ+KXzWqWWHp4gFPrzf3Sw/5zTLoF6iqBFIWQnk97RL22kvJQEMz63rqAThowqVEU4FjNE8iMFf931d7Gpwco6OJsDXok5eLsdK+l7Mfaa7/CD74uU3ghpfrHuw2MZKxI3OVHzOjVVpm1VvWbSjc8Y8ifUYlO2Ncqex8zCBjbfdypooZEIXj+BBQzUx1jqz5MA1jcVue43ItbkTTLmkWzC/Xqo/o2Vi3wI/ps9vsGrG99mUExX7yYltrJYCZlwMo0dh4Q5WUlhbMX196SCWc+Jag0LOiupo07C+0ssxJaQUN6nvpvc93vzF5lmI6T4sqCXalorCaKpuB/1uT09CHRlK9qVIxrmtMjDVFBruV3HuXk8ikXoOx+awke12kHCUkPir/vs8Zga1h9V5tFNw4Rjco9FUnKBbAl7vJ3BPXRhYIOB1j4uHwXXcIKtWpBjbgDMkBJjQfPyBm5DE1lX
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 206d02ec-2172-4cec-8f0f-08db52f68c39
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 May 2023 14:38:27.0966
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: FlgfjkSjEWOjNkX9ZG0CRBlB/dKg3wSZhHT56ey+LIDJ5tJApoX53UPONqXyTfD6bko/PM5XhTdYJrhQzNxZ9A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7125
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-12_08,2023-05-05_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=578 phishscore=0
 adultscore=0 spamscore=0 suspectscore=0 malwarescore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2304280000 definitions=main-2305120122
X-Proofpoint-ORIG-GUID: aIrAdeTkpKAhSqAjDxVdrY8mUuVcCcv-
X-Proofpoint-GUID: aIrAdeTkpKAhSqAjDxVdrY8mUuVcCcv-
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On May 12, 2023, at 7:30 AM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote=
:
>=20
> Thanks Chuck and Kees for the review.
>=20
>>> Actually netid should use the __string() and __assign_str()
>>> macros rather than open-coding a string copy, I think.
>=20
> Do you mean something like this?
>=20
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 4183819ea082..72a906a053dc 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1365,19 +1365,19 @@ TRACE_EVENT(nfsd_cb_setup,
>                __field(u32, cl_id)
>                __field(unsigned long, authflavor)
>                __sockaddr(addr, clp->cl_cb_conn.cb_addrlen)
> -               __array(unsigned char, netid, 8)
> +               __string(netid, netid)
>        ),
>        TP_fast_assign(
>                __entry->cl_boot =3D clp->cl_clientid.cl_boot;
>                __entry->cl_id =3D clp->cl_clientid.cl_id;
> -               strlcpy(__entry->netid, netid, sizeof(__entry->netid));
> +               __assign_str(netid, netid);
>                __entry->authflavor =3D authflavor;
>                __assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
>                                  clp->cl_cb_conn.cb_addrlen)
>        ),
>        TP_printk("addr=3D%pISpc client %08x:%08x proto=3D%s flavor=3D%s",
>                __get_sockaddr(addr), __entry->cl_boot, __entry->cl_id,
> -               __entry->netid, show_nfsd_authflavor(__entry->authflavor)=
)
> +               __get_str(netid), show_nfsd_authflavor(__entry->authflavo=
r))
> );

Yes. I don't remember why I chose to open-code the string
handling in here.

I agree with Kees, also: __assign_str() needs to handle
the memory accesses properly, and I had assumed it did
that already. My bad. That IMO should be handled as a
separate patch.


>>> Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
>=20
> Ack.

I agree that it's not a high priority backport. However,
I expect those who want to use trace points on KASAN-
enabled stable kernels might prefer to have the
__assign_str fix in place, along with this one.

So, I'm good with including Fixes: or leaving it off. Up
to you!

--
Chuck Lever