platform/x86: dell_rbu: Packet data fixes

Make fixes related to packet data list and buffers, and fix a sparse warning.

---
v4:
- Split patch relating to list into a fix patch and a refactor patch
- Add better explanation and "Fixes:" to buffer overwrite patch

v3:
- Cosmetic changes to commit messages
- Bump module version

v2:
- Add "__must_hold" to create_packet to remove sparse warning.
- Make struct packet_data_list static.

Stuart Hayes (5):
platform/x86: dell_rbu: Fix lock context warning platform/x86: dell_rbu: Fix list usage platform/x86: dell_rbu: Remove unused struct platform/x86: dell_rbu: Stop overwriting data buffer platform/x86: dell_rbu: Bump version

 drivers/platform/x86/dell/dell_rbu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

2.47.1

Fix a sparse lock context warning.

---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e30ca325938c..7b019fb72e86 100644
@@ -91,7 +91,7 @@ static void init_packet_head(void)
 	rbu_data.imagesize = 0;
 }
 
-static int create_packet(void *data, size_t length)
+static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
 {
 	struct packet_data *newpacket;
 	int ordernum = 0;

2.47.1

Pass the correct list head to list_for_each_entry*() when looping through the packet list.

Without this patch, reading the packet data via sysfs will show the data incorrectly (because it starts at the wrong packet), and clearing the packet list will result in a NULL pointer dereference.

---
 drivers/platform/x86/dell/dell_rbu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 7b019fb72e86..722979b19e0e 100644
@@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t * pread_length)
 	remaining_bytes = *pread_length;
 	bytes_read = rbu_data.packet_read_count;
 
-	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
+	list_for_each_entry(newpacket, &packet_data_head.list, list) {
 		bytes_copied = do_packet_read(pdest, newpacket,
 			remaining_bytes, bytes_read, &temp_count);
 		remaining_bytes -= bytes_copied;

@@ -315,7 +315,7 @@ static void packet_empty_list(void)
 {
 	struct packet_data *newpacket, *tmp;
 
-	list_for_each_entry_safe(newpacket, tmp, (&packet_data_head.list)->next, list) {
+	list_for_each_entry_safe(newpacket, tmp, &packet_data_head.list, list) {
 		list_del(&newpacket->list);
 
 		/*

2.47.1

Stop using an entire struct packet_data just for its embedded list_head.

---
 drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 722979b19e0e..c03d4d55fcc1 100644
@@ -77,14 +77,14 @@ struct packet_data {
 	int ordernum;
 };
 
-static struct packet_data packet_data_head;
+static struct list_head packet_data_list;
 
 static struct platform_device *rbu_device;
 static int context;
 
 static void init_packet_head(void)
 {
-	INIT_LIST_HEAD(&packet_data_head.list);
+	INIT_LIST_HEAD(&packet_data_list);
 	rbu_data.packet_read_count = 0;
 	rbu_data.num_packets = 0;
 	rbu_data.packetsize = 0;

@@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
 
 	/* initialize the newly created packet headers */
 	INIT_LIST_HEAD(&newpacket->list);
-	list_add_tail(&newpacket->list, &packet_data_head.list);
+	list_add_tail(&newpacket->list, &packet_data_list);
 
 	memcpy(newpacket->data, data, length);
 

@@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t * pread_length)
 	remaining_bytes = *pread_length;
 	bytes_read = rbu_data.packet_read_count;
 
-	list_for_each_entry(newpacket, &packet_data_head.list, list) {
+	list_for_each_entry(newpacket, &packet_data_list, list) {
 		bytes_copied = do_packet_read(pdest, newpacket,
 			remaining_bytes, bytes_read, &temp_count);
 		remaining_bytes -= bytes_copied;

@@ -315,7 +315,7 @@ static void packet_empty_list(void)
 {
 	struct packet_data *newpacket, *tmp;
 
-	list_for_each_entry_safe(newpacket, tmp, &packet_data_head.list, list) {
+	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
 		list_del(&newpacket->list);
 
 		/*

2.47.1

Thanks for the update.

I've applied all but this patch 3 now into the review-ilpo-fixes branch. As this patch is for-next material and I can only apply it once I've merged the fixes branch into the for-next branch somewhere later in the cycle (it'll sit in patchwork in the meantime, so I won't be forget it :-)).

--
i.

The dell_rbu driver will use memset() to clear the data held by each packet when it is no longer needed (when the driver is unloaded, the packet size is changed, etc).

The amount of memory that is cleared (before this patch) is the normal packet size. However, the last packet in the list may be smaller.

Fix this to only clear the memory actually used by each packet, to prevent it from writing past the end of data buffer.

Because the packet data buffers are allocated with __get_free_pages() (in page-sized increments), this bug could only result in a buffer being overwritten when a packet size larger than one page is used. The only user of the dell_rbu module should be the Dell BIOS update program, which uses a packet size of 4096, so no issues should be seen without the patch, it just blocks the possiblity.

---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c03d4d55fcc1..7d5b26735a20 100644
@@ -322,7 +322,7 @@ static void packet_empty_list(void)
 		 * zero out the RBU packet memory before freeing
 		 * to make sure there are no stale RBU packets left in memory
 		 */
-		memset(newpacket->data, 0, rbu_data.packetsize);
+		memset(newpacket->data, 0, newpacket->length);
 		set_memory_wb((unsigned long)newpacket->data,
 			1 << newpacket->ordernum);
 		free_pages((unsigned long) newpacket->data,

2.47.1

Bump the module version.

---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7d5b26735a20..45c0a72e494a 100644
@@ -45,7 +45,7 @@
 MODULE_AUTHOR("Abhay Salunke <[email protected]>");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
 MODULE_LICENSE("GPL");
-MODULE_VERSION("3.2");
+MODULE_VERSION("3.3");
 
 #define BIOS_SCAN_LIMIT 0xffffffff
 #define MAX_IMAGE_LENGTH 16

2.47.1

Thank you for your contribution, it has been applied to my local review-ilpo-next branch. Note it will show up in the public platform-drivers-x86/review-ilpo-next branch only once I've pushed my local branch there, which might take a while.

The list of commits applied:
[1/5] platform/x86: dell_rbu: Fix lock context warning
(no commit info)
[2/5] platform/x86: dell_rbu: Fix list usage
(no commit info)
[3/5] platform/x86: dell_rbu: Remove unused struct

      commit: 56036d6af41a473a8129fc960a5ab3673eda13d5

[4/5] platform/x86: dell_rbu: Stop overwriting data buffer
(no commit info)
[5/5] platform/x86: dell_rbu: Bump version
(no commit info)

--
i.