Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
From: Gustavo A. R. Silva <hidden>
Date: 2021-03-26 04:30:22
Also in:
linux-hardening, lkml
Hi Martin, On 3/25/21 22:34, Martin K. Petersen wrote:
quoted hunk ↗ jump to hunk
Gustavo,quoted
Precisely this sort of confusion is one of the things we want to avoid by using flexible-array members instead of one-element arrays.Ah, you're right! Now that I look at it again I also don't think that was the issue that originally caused concern.@@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int } } sge[pos] = rio2->sge[nseg-1]; - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); + memcpy(&rio2->sge[1], &sge[1], + flex_array_size(rio2, sge, nseg_new - 1)); kfree(sge); rio2->sgeCnt = cpu_to_le32(nseg_new);I find it counter-intuitive to use the type of the destination array to size the amount of source data to copy. "Are source and destination same
The destination and source arrays are of the same type. :) drivers/scsi/aacraid/aachba.c: 3999 struct sge_ieee1212 *sge;
type? Does flex_array_size() do the right thing given the ->sge[1] destination offset?". It wasn't immediately obvious. To me, "copy this many scatterlist entries" in the original is much more readable.
Yeah; it does the right thing because flex_array_size() doesn't know about offsets. It just calculates the amount of bytes to be copied based on the type of the object passed as second argument and a "count" passed as third argument. So, in this case, the "count" is "nseg_new - 1", which in some way is already taking care of that sge[1] offset. -- Gustavo