| Message ID | c80c0979933e0c05e80d95792ef167a28640a14b.1663816572.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: smartpqi: Replace one-element arrays with flexible-array members | expand |
On Wed, Sep 21, 2022 at 11:28:35PM -0500, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element array with flexible-array > member in struct MR_DRV_RAID_MAP and refactor the the rest of the code > accordingly. > > It seems that the addition of sizeof(struct report_log_lun) in all the > places that are modified by this patch is due to the fact that > the one-element array struct report_log_lun lun_entries[1]; always > contributes to the size of the containing structure struct > report_log_lun_list. > > Notice that at line 1267 while allocating memory for an instance of > struct report_log_lun_list, some _extra_ space seems to be allocated > for one element of type struct report_log_lun, which is the type of > the elements in array lun_entries: > > 1267 internal_logdev_list = kmalloc(logdev_data_length + > 1268 sizeof(struct report_log_lun), GFP_KERNEL); > > However, at line 1275 just logdev_data_length bytes are copied into > internal_logdev_list (remember that we allocated space for logdev_data_length + > sizeof(struct report_log_lun) bytes at line 1267), and then exactly > sizeof(struct report_log_lun) bytes are being zeroing out at line 1276. > > 1275 memcpy(internal_logdev_list, logdev_data, logdev_data_length); > 1276 memset((u8 *)internal_logdev_list + logdev_data_length, 0, > 1277 sizeof(struct report_log_lun)); > > All the above makes think that it's just fine if we transform array > lun_entries into a flexible-array member and just don't allocate > that extra sizeof(struct report_log_lun) bytes of space. With this > we can remove that memset() call and we also need to modify the code > that updates the total length (internal_logdev_list->header.list_length) > of array lun_entries at line 1278: > > 1278 put_unaligned_be32(logdev_list_length + > 1279 sizeof(struct report_log_lun), > 1280 &internal_logdev_list->header.list_length); > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines > on memcpy(). > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/204 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > And of course, it'd be great if maintainers can confirm what I described > in the changelog text. :) > > drivers/scsi/smartpqi/smartpqi.h | 2 +- > drivers/scsi/smartpqi/smartpqi_init.c | 10 +++------- > 2 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h > index e550b12e525a..d1756c9d1112 100644 > --- a/drivers/scsi/smartpqi/smartpqi.h > +++ b/drivers/scsi/smartpqi/smartpqi.h > @@ -954,7 +954,7 @@ struct report_log_lun { > > struct report_log_lun_list { > struct report_lun_header header; > - struct report_log_lun lun_entries[1]; > + struct report_log_lun lun_entries[]; > }; > > struct report_phys_lun_8byte_wwid { > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index b971fbe3b3a1..544cd18a90d7 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -1264,8 +1264,7 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, > logdev_data_length = sizeof(struct report_lun_header) + > logdev_list_length; > > - internal_logdev_list = kmalloc(logdev_data_length + > - sizeof(struct report_log_lun), GFP_KERNEL); > + internal_logdev_list = kmalloc(logdev_data_length, GFP_KERNEL); > if (!internal_logdev_list) { > kfree(*logdev_list); > *logdev_list = NULL; > @@ -1273,11 +1272,8 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, > } > > memcpy(internal_logdev_list, logdev_data, logdev_data_length); > - memset((u8 *)internal_logdev_list + logdev_data_length, 0, > - sizeof(struct report_log_lun)); > - put_unaligned_be32(logdev_list_length + > - sizeof(struct report_log_lun), > - &internal_logdev_list->header.list_length); > + put_unaligned_be32(logdev_list_length, > + &internal_logdev_list->header.list_length); This is a bit of a twisty maze to read through, but at the end, I agree with your assessment: it was needlessly added the extra member to the allocation. I don't see it used anywhere in later code -- it's always bounded by logdev_list_length. Reviewed-by: Kees Cook <keescook@chromium.org> > > kfree(*logdev_list); > *logdev_list = internal_logdev_list; > -- > 2.34.1 >
On 2/6/23 15:58, Don.Brace@microchip.com wrote: > > ________________________________ > From: Gustavo A. R. Silva <gustavoars@kernel.org> > Sent: Wednesday, September 21, 2022 11:28 PM > To: Kevin Barnett <kevin.barnett@microsemi.com>; Don Brace - C33706 <Don.Brace@microchip.com>; storagedev <storagedev@microchip.com>; James E.J. Bottomley <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org <linux-scsi@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Gustavo A. R. Silva <gustavoars@kernel.org>; linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org> > Subject: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member > > [Some people who received this message don't often get email from gustavoars@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element array with flexible-array > member in struct MR_DRV_RAID_MAP and refactor the the rest of the code > accordingly. > > It seems that the addition of sizeof(struct report_log_lun) in all the > places that are modified by this patch is due to the fact that > the one-element array struct report_log_lun lun_entries[1]; always > contributes to the size of the containing structure struct > report_log_lun_list. > > Notice that at line 1267 while allocating memory for an instance of > struct report_log_lun_list, some _extra_ space seems to be allocated > for one element of type struct report_log_lun, which is the type of > the elements in array lun_entries: > > 1267 internal_logdev_list = kmalloc(logdev_data_length + > 1268 sizeof(struct report_log_lun), GFP_KERNEL); > > However, at line 1275 just logdev_data_length bytes are copied into > internal_logdev_list (remember that we allocated space for logdev_data_length + > sizeof(struct report_log_lun) bytes at line 1267), and then exactly > sizeof(struct report_log_lun) bytes are being zeroing out at line 1276. > > 1275 memcpy(internal_logdev_list, logdev_data, logdev_data_length); > 1276 memset((u8 *)internal_logdev_list + logdev_data_length, 0, > 1277 sizeof(struct report_log_lun)); > > All the above makes think that it's just fine if we transform array > lun_entries into a flexible-array member and just don't allocate > that extra sizeof(struct report_log_lun) bytes of space. With this > we can remove that memset() call and we also need to modify the code > that updates the total length (internal_logdev_list->header.list_length) > of array lun_entries at line 1278: > > 1278 put_unaligned_be32(logdev_list_length + > 1279 sizeof(struct report_log_lun), > 1280 &internal_logdev_list->header.list_length); > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines > on memcpy(). > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/204 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > NAK: What is actually happening is that we were taking on an extra list entry that is all zeros for the controller itself. This is intentional. These changes will break the driver. Oh, great to know. :) So, in this case, what do you think about this, instead: diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h index af27bb0f3133..228838eb3686 100644 --- a/drivers/scsi/smartpqi/smartpqi.h +++ b/drivers/scsi/smartpqi/smartpqi.h @@ -954,7 +954,7 @@ struct report_log_lun { struct report_log_lun_list { struct report_lun_header header; - struct report_log_lun lun_entries[1]; + struct report_log_lun lun_entries[]; }; struct report_phys_lun_8byte_wwid { diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index d0446d4d4465..af8f1a8e9f8f 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -1277,6 +1277,10 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, logdev_data_length = sizeof(struct report_lun_header) + logdev_list_length; + /* + * Notice that we take on an extra list entry (struct report_log_lun) + * that is all zeros for the controller itself. + */ internal_logdev_list = kmalloc(logdev_data_length + sizeof(struct report_log_lun), GFP_KERNEL); if (!internal_logdev_list) { Thanks for the feedback! -- Gustavo > > Thanks, > Don Brace <don.brace@microchip.com> > > > --- > And of course, it'd be great if maintainers can confirm what I described > in the changelog text. :) > > drivers/scsi/smartpqi/smartpqi.h | 2 +- > drivers/scsi/smartpqi/smartpqi_init.c | 10 +++------- > 2 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h > index e550b12e525a..d1756c9d1112 100644 > --- a/drivers/scsi/smartpqi/smartpqi.h > +++ b/drivers/scsi/smartpqi/smartpqi.h > @@ -954,7 +954,7 @@ struct report_log_lun { > > struct report_log_lun_list { > struct report_lun_header header; > - struct report_log_lun lun_entries[1]; > + struct report_log_lun lun_entries[]; > }; > > struct report_phys_lun_8byte_wwid { > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index b971fbe3b3a1..544cd18a90d7 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -1264,8 +1264,7 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, > logdev_data_length = sizeof(struct report_lun_header) + > logdev_list_length; > > - internal_logdev_list = kmalloc(logdev_data_length + > - sizeof(struct report_log_lun), GFP_KERNEL); > + internal_logdev_list = kmalloc(logdev_data_length, GFP_KERNEL); > if (!internal_logdev_list) { > kfree(*logdev_list); > *logdev_list = NULL; > @@ -1273,11 +1272,8 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, > } > > memcpy(internal_logdev_list, logdev_data, logdev_data_length); > - memset((u8 *)internal_logdev_list + logdev_data_length, 0, > - sizeof(struct report_log_lun)); > - put_unaligned_be32(logdev_list_length + > - sizeof(struct report_log_lun), > - &internal_logdev_list->header.list_length); > + put_unaligned_be32(logdev_list_length, > + &internal_logdev_list->header.list_length); > > kfree(*logdev_list); > *logdev_list = internal_logdev_list; > -- > 2.34.1 > >
On 2/7/23 15:08, Don.Brace@microchip.com wrote: > > ________________________________ > From: Gustavo A. R. Silva <gustavo@embeddedor.com> > Sent: Monday, February 6, 2023 4:28 PM > To: Don Brace - C33706 <Don.Brace@microchip.com>; gustavoars@kernel.org <gustavoars@kernel.org>; kevin.barnett@microsemi.com <kevin.barnett@microsemi.com>; storagedev <storagedev@microchip.com>; jejb@linux.ibm.com <jejb@linux.ibm.com>; martin.petersen@oracle.com <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org <linux-scsi@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org>; Kees Cook <keescook@chromium.org> > Subject: Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member > > >> NAK: What is actually happening is that we were taking on an extra list entry that is all zeros for the controller itself. This is intentional. These changes will break the driver. > > Oh, great to know. :) > > So, in this case, what do you think about this, instead: > > diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h > index af27bb0f3133..228838eb3686 100644 > --- a/drivers/scsi/smartpqi/smartpqi.h > +++ b/drivers/scsi/smartpqi/smartpqi.h > @@ -954,7 +954,7 @@ struct report_log_lun { > > struct report_log_lun_list { > struct report_lun_header header; > - struct report_log_lun lun_entries[1]; > + struct report_log_lun lun_entries[]; > }; > > This HUNK is OK. > Thanks for your patch. > > Acked-by: Don Brace <don.brace@microchip.com> > Don > > struct report_phys_lun_8byte_wwid { > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c > index d0446d4d4465..af8f1a8e9f8f 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -1277,6 +1277,10 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, > logdev_data_length = sizeof(struct report_lun_header) + > logdev_list_length; > > + /* > + * Notice that we take on an extra list entry (struct report_log_lun) > + * that is all zeros for the controller itself. > + */ > internal_logdev_list = kmalloc(logdev_data_length + > sizeof(struct report_log_lun), GFP_KERNEL); > if (!internal_logdev_list) { > > The driver author has provided his input on the change to the above comment: > > I think that'd I'd prefer that we just amplify the existing comment to something like this: > > /* > * Tack the controller itself onto the end of the logical device list > * by adding a list entry that is all zeros. > */ OK. Great. :) Thanks -- Gustavo > > Thank-you for your patch > Don Brace <don.brace@microchip.com> > >
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h index e550b12e525a..d1756c9d1112 100644 --- a/drivers/scsi/smartpqi/smartpqi.h +++ b/drivers/scsi/smartpqi/smartpqi.h @@ -954,7 +954,7 @@ struct report_log_lun { struct report_log_lun_list { struct report_lun_header header; - struct report_log_lun lun_entries[1]; + struct report_log_lun lun_entries[]; }; struct report_phys_lun_8byte_wwid { diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index b971fbe3b3a1..544cd18a90d7 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -1264,8 +1264,7 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, logdev_data_length = sizeof(struct report_lun_header) + logdev_list_length; - internal_logdev_list = kmalloc(logdev_data_length + - sizeof(struct report_log_lun), GFP_KERNEL); + internal_logdev_list = kmalloc(logdev_data_length, GFP_KERNEL); if (!internal_logdev_list) { kfree(*logdev_list); *logdev_list = NULL; @@ -1273,11 +1272,8 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info, } memcpy(internal_logdev_list, logdev_data, logdev_data_length); - memset((u8 *)internal_logdev_list + logdev_data_length, 0, - sizeof(struct report_log_lun)); - put_unaligned_be32(logdev_list_length + - sizeof(struct report_log_lun), - &internal_logdev_list->header.list_length); + put_unaligned_be32(logdev_list_length, + &internal_logdev_list->header.list_length); kfree(*logdev_list); *logdev_list = internal_logdev_list;
One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace one-element array with flexible-array member in struct MR_DRV_RAID_MAP and refactor the the rest of the code accordingly. It seems that the addition of sizeof(struct report_log_lun) in all the places that are modified by this patch is due to the fact that the one-element array struct report_log_lun lun_entries[1]; always contributes to the size of the containing structure struct report_log_lun_list. Notice that at line 1267 while allocating memory for an instance of struct report_log_lun_list, some _extra_ space seems to be allocated for one element of type struct report_log_lun, which is the type of the elements in array lun_entries: 1267 internal_logdev_list = kmalloc(logdev_data_length + 1268 sizeof(struct report_log_lun), GFP_KERNEL); However, at line 1275 just logdev_data_length bytes are copied into internal_logdev_list (remember that we allocated space for logdev_data_length + sizeof(struct report_log_lun) bytes at line 1267), and then exactly sizeof(struct report_log_lun) bytes are being zeroing out at line 1276. 1275 memcpy(internal_logdev_list, logdev_data, logdev_data_length); 1276 memset((u8 *)internal_logdev_list + logdev_data_length, 0, 1277 sizeof(struct report_log_lun)); All the above makes think that it's just fine if we transform array lun_entries into a flexible-array member and just don't allocate that extra sizeof(struct report_log_lun) bytes of space. With this we can remove that memset() call and we also need to modify the code that updates the total length (internal_logdev_list->header.list_length) of array lun_entries at line 1278: 1278 put_unaligned_be32(logdev_list_length + 1279 sizeof(struct report_log_lun), 1280 &internal_logdev_list->header.list_length); This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy(). Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/204 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- And of course, it'd be great if maintainers can confirm what I described in the changelog text. :) drivers/scsi/smartpqi/smartpqi.h | 2 +- drivers/scsi/smartpqi/smartpqi_init.c | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-)