Re: [PATCH net-next] net/tls: Mark the end in scatterlist table
From: Vakul Garg Date: Thu, 2 Aug 2018 20:43:10 +0530 > Function zerocopy_from_iter() unmarks the 'end' in input sgtable while > adding new entries in it. The last entry in sgtable remained unmarked. > This results in KASAN error report on using apis like sg_nents(). Before > returning, the function needs to mark the 'end' in the last entry it > adds. > > Signed-off-by: Vakul Garg Yes, a properly formed scatterlist table must always have it's end marked. Applied, thanks.
RE: [PATCH net-next] net/tls: Mark the end in scatterlist table
> -Original Message- > From: Dave Watson [mailto:davejwat...@fb.com] > Sent: Thursday, August 2, 2018 10:47 PM > To: Vakul Garg > Cc: netdev@vger.kernel.org; bor...@mellanox.com; > avia...@mellanox.com; da...@davemloft.net > Subject: Re: [PATCH net-next] net/tls: Mark the end in scatterlist table > > On 08/02/18 05:05 PM, Vakul Garg wrote: > > In case zerocopy_from_iter() fails, 'end' won't get marked. > > So fallback path is fine. > > > > > Which codepath is calling sg_nents()? > > > > While testing my WIP implementation of combined dynamic memory > > allocation for (aead_req || sgin || sgout || aad || iv), I was getting > random kernel crashes. > > To debug it I had inserted sg_nents() in my code. The KASAN then > > immediately complained that sg_nents() went beyond the allocated > memory for scatterlist. > > This led me to find that scatterlist table end has not been marked. > > > > If this isn't causing KASAN issues for the existing code, it probably makes > more sense to put in a future series with that WIP work then. Isn't using a sgtable with unmarked end already bad and should be fixed? Crypto hardware accelerator drivers could be broken while using sg lists with unmarked end.
Re: [PATCH net-next] net/tls: Mark the end in scatterlist table
On 08/02/18 05:05 PM, Vakul Garg wrote: > In case zerocopy_from_iter() fails, 'end' won't get marked. > So fallback path is fine. > > > Which codepath is calling sg_nents()? > > While testing my WIP implementation of combined dynamic memory allocation for > (aead_req || sgin || sgout || aad || iv), I was getting random kernel crashes. > To debug it I had inserted sg_nents() in my code. The KASAN then immediately > complained that sg_nents() went beyond the allocated memory for scatterlist. > This led me to find that scatterlist table end has not been marked. > If this isn't causing KASAN issues for the existing code, it probably makes more sense to put in a future series with that WIP work then.
RE: [PATCH net-next] net/tls: Mark the end in scatterlist table
> -Original Message- > From: Dave Watson [mailto:davejwat...@fb.com] > Sent: Thursday, August 2, 2018 10:17 PM > To: Vakul Garg > Cc: netdev@vger.kernel.org; bor...@mellanox.com; > avia...@mellanox.com; da...@davemloft.net > Subject: Re: [PATCH net-next] net/tls: Mark the end in scatterlist table > > On 08/02/18 08:43 PM, Vakul Garg wrote: > > Function zerocopy_from_iter() unmarks the 'end' in input sgtable while > > adding new entries in it. The last entry in sgtable remained unmarked. > > This results in KASAN error report on using apis like sg_nents(). > > Before returning, the function needs to mark the 'end' in the last > > entry it adds. > > > > Signed-off-by: Vakul Garg > > Looks good to me, it looks like the fallback path should unmark the end > appropriately. > In case zerocopy_from_iter() fails, 'end' won't get marked. So fallback path is fine. > Which codepath is calling sg_nents()? While testing my WIP implementation of combined dynamic memory allocation for (aead_req || sgin || sgout || aad || iv), I was getting random kernel crashes. To debug it I had inserted sg_nents() in my code. The KASAN then immediately complained that sg_nents() went beyond the allocated memory for scatterlist. This led me to find that scatterlist table end has not been marked. > Acked-by: Dave Watson
Re: [PATCH net-next] net/tls: Mark the end in scatterlist table
On 08/02/18 08:43 PM, Vakul Garg wrote: > Function zerocopy_from_iter() unmarks the 'end' in input sgtable while > adding new entries in it. The last entry in sgtable remained unmarked. > This results in KASAN error report on using apis like sg_nents(). Before > returning, the function needs to mark the 'end' in the last entry it > adds. > > Signed-off-by: Vakul Garg Looks good to me, it looks like the fallback path should unmark the end appropriately. Which codepath is calling sg_nents()? Acked-by: Dave Watson
