Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace 1-element arrays in drivers/scsi/smartpqi/ #204

Closed
GustavoARSilva opened this issue Sep 22, 2022 · 1 comment
Closed

Replace 1-element arrays in drivers/scsi/smartpqi/ #204

GustavoARSilva opened this issue Sep 22, 2022 · 1 comment
Assignees
@GustavoARSilva
Labels
[Idiom] fake flexible array [PATCH] Exists A patch exists to address the issue [Refactor] 1-element array Conversion away from one-element array

Comments

@GustavoARSilva
Copy link
Collaborator

GustavoARSilva commented Sep 22, 2022 •
edited

drivers/scsi/smartpqi/smartpqi.h:

 955 struct report_log_lun_list {                                                                        
 956         struct report_lun_header header;                                                            
 957         struct report_log_lun lun_entries[1];                                                       
 958 };
...
 983 struct report_phys_lun_8byte_wwid_list {                                                            
 984         struct report_lun_header header;                                                            
 985         struct report_phys_lun_8byte_wwid lun_entries[1];                                           
 986 };                                                                                                  
 987                                                                                                     
 988 struct report_phys_lun_16byte_wwid_list {                                                           
 989         struct report_lun_header header;                                                            
 990         struct report_phys_lun_16byte_wwid lun_entries[1];                                          
 991 };

Audit (at least) all these places where the flex array is being used:

diff -u -p drivers/scsi/smartpqi/smartpqi_init.c /tmp/nothing/smartpqi/smartpqi_init.c
--- drivers/scsi/smartpqi/smartpqi_init.c
+++ /tmp/nothing/smartpqi/smartpqi_init.c
@@ -1191,7 +1191,6 @@ static inline int pqi_report_phys_luns(s
        }

        rpl_8byte_wwid_list = rpl_list;
-       num_physicals = get_unaligned_be32(&rpl_8byte_wwid_list->header.list_length) / sizeof(rpl_8byte_wwid_list->lun_entries[0]);
        rpl_16byte_wwid_list_length = sizeof(struct report_lun_header) + (num_physicals * sizeof(struct report_phys_lun_16byte_wwid));

        rpl_16byte_wwid_list = kmalloc(rpl_16byte_wwid_list_length, GFP_KERNEL);
@@ -1203,14 +1202,6 @@ static inline int pqi_report_phys_luns(s
        rpl_16byte_wwid_list->header.flags = rpl_8byte_wwid_list->header.flags;

        for (i = 0; i < num_physicals; i++) {
-               memcpy(&rpl_16byte_wwid_list->lun_entries[i].lunid, &rpl_8byte_wwid_list->lun_entries[i].lunid, sizeof(rpl_8byte_wwid_list->lun_entries[i].lunid));
-               memcpy(&rpl_16byte_wwid_list->lun_entries[i].wwid[0], &rpl_8byte_wwid_list->lun_entries[i].wwid, sizeof(rpl_8byte_wwid_list->lun_entries[i].wwid));
-               memset(&rpl_16byte_wwid_list->lun_entries[i].wwid[8], 0, 8);
-               rpl_16byte_wwid_list->lun_entries[i].device_type = rpl_8byte_wwid_list->lun_entries[i].device_type;
-               rpl_16byte_wwid_list->lun_entries[i].device_flags = rpl_8byte_wwid_list->lun_entries[i].device_flags;
-               rpl_16byte_wwid_list->lun_entries[i].lun_count = rpl_8byte_wwid_list->lun_entries[i].lun_count;
-               rpl_16byte_wwid_list->lun_entries[i].redundant_paths = rpl_8byte_wwid_list->lun_entries[i].redundant_paths;
-               rpl_16byte_wwid_list->lun_entries[i].aio_handle = rpl_8byte_wwid_list->lun_entries[i].aio_handle;
        }

        kfree(rpl_8byte_wwid_list);
@@ -2369,14 +2360,12 @@ static int pqi_update_scsi_devices(struc
        if (physdev_list)
                num_physicals =
                        get_unaligned_be32(&physdev_list->header.list_length)
-                               / sizeof(physdev_list->lun_entries[0]);
        else
                num_physicals = 0;

        if (logdev_list)
                num_logicals =
                        get_unaligned_be32(&logdev_list->header.list_length)
-                               / sizeof(logdev_list->lun_entries[0]);
        else
                num_logicals = 0;

@@ -2397,7 +2386,6 @@ static int pqi_update_scsi_devices(struc

                if (pqi_hide_vsep) {
                        for (i = num_physicals - 1; i >= 0; i--) {
-                               phys_lun = &physdev_list->lun_entries[i];
                                if (CISS_GET_DRIVE_NUMBER(phys_lun->lunid) == PQI_VSEP_CISS_BTL) {
                                        pqi_mask_device(phys_lun->lunid);
                                        break;
@@ -2443,13 +2431,11 @@ static int pqi_update_scsi_devices(struc
                if ((!pqi_expose_ld_first && i < num_physicals) ||
                        (pqi_expose_ld_first && i >= num_logicals)) {
                        is_physical_device = true;
-                       phys_lun = &physdev_list->lun_entries[physical_index++];
                        log_lun = NULL;
                        scsi3addr = phys_lun->lunid;
                } else {
                        is_physical_device = false;
                        phys_lun = NULL;
-                       log_lun = &logdev_list->lun_entries[logical_index++];
                        scsi3addr = log_lun->lunid;
                }
@GustavoARSilva GustavoARSilva self-assigned this Sep 22, 2022
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 22, 2022
@GustavoARSilva @intel-lab-lkp
scsi: smartpqi: Replace one-element array with flexible-array member
1ac46d1 
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: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 22, 2022
@GustavoARSilva @intel-lab-lkp
scsi: smartpqi: Replace one-element arrays with flexible-array members
62cb287 
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 structures report_phys_lun_8byte_wwid_list and
report_phys_lun_16byte_wwid_list.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().

Link: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
@GustavoARSilva GustavoARSilva added [Refactor] 1-element array Conversion away from one-element array [PATCH] Exists A patch exists to address the issue labels Sep 22, 2022
@GustavoARSilva
Copy link
Collaborator Author

Patch series for this:
https://lore.kernel.org/linux-hardening/cover.1663816572.git.gustavoars@kernel.org/

@GustavoARSilva GustavoARSilva added [PATCH] Accepted A submitted patch has been accepted upstream and removed [PATCH] Exists A patch exists to address the issue labels Oct 7, 2022
@GustavoARSilva GustavoARSilva reopened this Feb 7, 2023
@GustavoARSilva GustavoARSilva removed the [PATCH] Accepted A submitted patch has been accepted upstream label Feb 7, 2023
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Feb 7, 2023
@GustavoARSilva @intel-lab-lkp
scsi: smartpqi: Replace one-element array with flexible-array member
61f9ebd 
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 report_log_lun_list.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().

Link: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Feb 7, 2023
@GustavoARSilva @intel-lab-lkp
scsi: smartpqi: Replace one-element array with flexible-array member
f92f3f9 
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 report_log_lun_list.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().

Link: KSPP#79
Link: KSPP#204
Acked-by: Don Brace <don.brace@microchip.com>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
@GustavoARSilva GustavoARSilva added the [PATCH] Exists A patch exists to address the issue label Feb 9, 2023
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Feb 9, 2023
@GustavoARSilva @martinkpetersen
scsi: smartpqi: Replace one-element array with flexible-array member
ead8212 
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 report_log_lun_list.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().

Link: https://lore.kernel.org/r/Y+LJz/r6+UeLqnV3@work
Link: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Acked-by: Don Brace <don.brace@microchip.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Jun 21, 2023
@GustavoARSilva @intel-lab-lkp
scsi: smartpqi: Replace one-element arrays with flexible-array members
6a0c9ea 
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element arrays with flexible-array
members in a couple of structures, and refactor the rest of the code,
accordingly.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy().

This results in no differences in binary output.

Link: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
akiyks pushed a commit to akiyks/linux that referenced this issue Jun 22, 2023
@GustavoARSilva @martinkpetersen
scsi: smartpqi: Replace one-element arrays with flexible-array members
6f0a92f 
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element arrays with flexible-array
members in a couple of structures, and refactor the rest of the code,
accordingly.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().

This results in no differences in binary output.

Link: KSPP#79
Link: KSPP#204
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Link: https://lore.kernel.org/r/ZJNdKDkuRbFZpASS@work
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@GustavoARSilva GustavoARSilva

Labels
[Idiom] fake flexible array [PATCH] Exists A patch exists to address the issue [Refactor] 1-element array Conversion away from one-element array
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@GustavoARSilva